-
Notifications
You must be signed in to change notification settings - Fork 573
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
feat: improve artwork lists performance (attempt 1) #8782
feat: improve artwork lists performance (attempt 1) #8782
Conversation
</Join> | ||
</Flex> | ||
</TouchableOpacity> | ||
) | ||
} | ||
|
||
export const ArtworkListItem = memo(Item) |
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.
re-render the component only when the props have changed
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.
Looks good, just a TS question below…
@@ -12,9 +13,13 @@ interface ArtworkListsProps { | |||
me: ArtworkLists_me$key | null | |||
} | |||
|
|||
type ArtworkList = | |||
| NonNullable<ArtworkLists_me$data["savedArtworksArtworkList"]> | |||
| ExtractNodeType<ArtworkLists_me$data["savedArtworksArtworkList"]> |
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.
I'm unclear about what this type is representing. Is L18 L17 not sufficient?
(The ExtractNodeType
utility seems to be meant for use with collections only?)
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.
Is L18 not sufficient
it will be enough, but it seems to me it would be more correct to use the type of "Saved Artworks" and Custom Artwork lists
my bad, i forgot to update some code
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.
The ExtractNodeType utility seems to be meant for use with collections only
yes, for cases when the query has edges -> node
. For example
{
someConnection {
edges {
node {
...
}
}
}
}
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.
Done: c7f2a0d
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.
👍🏽
This PR resolves []
Notion Card: https://www.notion.so/artsy/Selecting-unselecting-lists-in-the-sheet-can-feel-a-bit-sluggish-Takes-a-second-or-two-to-settle-b63ab6d694dc4f8e94ccefda22386b84
Description
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.