-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GAL-5440] add Suggested Profiles in curated Feed web #2429
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Bundle SizesCompared against b120793
|
@@ -89,6 +89,7 @@ export default function CuratedFeed({ queryRef }: Props) { | |||
loadNextPage={loadNextPage} | |||
hasNext={hasPrevious} | |||
feedMode={'WORLDWIDE'} | |||
showSuggestedProfiles={true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can just pass in showSuggestedProfiles
without setting to true
feedMode={'WORLDWIDE'}
showSuggestedProfiles
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the change!
const suggestedProfileSectionHeight = useMemo( | ||
() => (isMobileOrMobileLargeWindowWidth ? 320 : 360), | ||
[isMobileOrMobileLargeWindowWidth] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: don't need to memoize primitive types; memoization mostly meant for complex types like computed values, arrays, objects.
can just do:
const suggestedProfileSectionHeight = isMobileOrMobileLargeWindowWidth ? 320 : 360
if (showSuggestedProfiles && feedData?.length >= suggestedProfileSectionIdx) { | ||
const suggestedProfileSectionData = { | ||
__typename: 'SuggestedProfileSection', | ||
dbid: '12345', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use uuid()
, we use it in a couple other places in the repo
const rowCount = hasNext ? feedData.length + 1 : feedData.length; | ||
const rowCount = hasNext ? finalFeedData.length + 1 : finalFeedData.length; | ||
|
||
const rowHeight = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work figuring this out!
const path = { | ||
pathname: '/[username]', | ||
query: { username: username }, | ||
} as Route; | ||
|
||
if (!path) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we need the check on L64-66 if path
is clearly defined on L59?
queryRef | ||
); | ||
return ( | ||
<ReportingErrorBoundary fallback={<></>}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have skeleton states?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
adds a new item (the Suggested Profiles section) in the
Curated
Feed after the 8th feed list item.Demo or Before/After Pics
Edge Cases
Moweb:
Testing Steps
Provide steps on how the reviewer can test the changes.
Checklist
Please make sure to review and check all of the following: