- 
                Notifications
    You must be signed in to change notification settings 
- Fork 393
Update to new card design #4065
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
Conversation
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.
First pass: I only know what this PR is about because I read the branch name. Can you please update the description?
| const pack = selectedNodePack.value | ||
| if (!pack?.id) return | ||
| if (hasMultipleSelections.value) return | ||
| const data = await getPackById.call(selectedNodePack.value.id) | ||
| if (data?.id === selectedNodePack.value?.id) { | ||
| // If selected node hasn't changed since request, merge registry & Algolia data | ||
| selectedNodePacks.value = [merge(selectedNodePack.value, data)] | ||
| // Only fetch if we haven't already for this pack | ||
| if (lastFetchedPackId.value === pack.id) return | ||
| const data = await getPackById.call(pack.id) | ||
| // If selected node hasn't changed since request, merge registry & Algolia data | ||
| if (data?.id === pack.id) { | ||
| lastFetchedPackId.value = pack.id | ||
| const mergedPack = merge({}, pack, data) | ||
| selectedNodePacks.value = [mergedPack] | ||
| // Replace pack in displayPacks so that children receive a fresh prop reference | ||
| const idx = displayPacks.value.findIndex((p) => p.id === mergedPack.id) | ||
| if (idx !== -1) { | ||
| displayPacks.value.splice(idx, 1, mergedPack) | ||
| } | 
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.
Can you verify using Network tab in dev tools that this caching is necessary? The getPackById should already be wrapped with useCachedRequest which handles request caching / deduplication. Although in general this doesn't really hurt to add, so not a big deal.
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.
apart from caching, it also prevents an infinite loop issue I had, it should be OK to ship as is, not too important
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.
SGTM
This reverts commit 228b4d4.
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!
Co-authored-by: filtered <[email protected]>
This PR affects the new manager, by changing the design of the new manager's cards to be up to date with the latest Figma reference.
However, please note that banner_url and github stars are both missing from this PR. They'll both be added in a future patch, once the backend is updated.
┆Issue is synchronized with this Notion page by Unito