-
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
fix(CX-2543): Fix performance with my collection search #6676
fix(CX-2543): Fix performance with my collection search #6676
Conversation
…x-cx-2543-fix-performance-my-collection-search
…x-cx-2543-fix-performance-my-collection-search
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 is great work! 🌟 I've tested it on the simulator and it works very well. I'll approve but please let someone else review as well before merging.
return React.isValidElement(HeaderComponent) ? ( | ||
<View onLayout={this.onHeaderLayout}>{HeaderComponent}</View> | ||
) : ( | ||
<View onLayout={this.onHeaderLayout}> | ||
<HeaderComponent /> | ||
</View> | ||
) |
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 we just check for the type of the header component and change this to:
return React.isValidElement(HeaderComponent) ? ( | |
<View onLayout={this.onHeaderLayout}>{HeaderComponent}</View> | |
) : ( | |
<View onLayout={this.onHeaderLayout}> | |
<HeaderComponent /> | |
</View> | |
) | |
return ( | |
<View onLayout={this.onHeaderLayout}> | |
<HeaderComponent /> | |
</View> | |
) |
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.
@olerichter00 I think it reduces ambiguity. Reason being that this allows for devs to pass () => </> or Element as is. Typically this Header should be in a ListHeaderComponent where you would pass element that passes React.isValidElement
, but it isn't because of how the grid is composed. I think the original authors wanted devs to have the freedom to use it as they would typically use a FlatList's ListHeaderComponent and I think that makes a lot of sense. I only wrapped a view around it to measure the height.
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.
Ok
return React.isValidElement(HeaderComponent) ? ( | ||
<View onLayout={onHeaderLayout}>{HeaderComponent}</View> | ||
) : ( | ||
<View onLayout={onHeaderLayout}> | ||
<HeaderComponent /> | ||
</View> | ||
) |
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.
Same comment as above.
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.
@olerichter00 This only emulates what's in InfiniteGridArtworks
so they stay consistent since we use both same way.
src/app/Scenes/MyCollection/Components/MyCollectionArtworkList.tsx
Outdated
Show resolved
Hide resolved
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.
Niice thanks a lot Kizito!
This PR resolves CX-2543
Description
Previous Issues this PR fixes:
What was done in this PR:
Possible Remaining issues:
coll-search-ios.mp4
coll-search-android.mp4
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 on our docs, or get in touch with us.