-
Notifications
You must be signed in to change notification settings - Fork 583
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(perf): search perf #11096
feat(perf): search perf #11096
Conversation
@@ -87,6 +91,7 @@ export const EntitySearchResults: React.FC<SearchResultsProps> = ({ query, selec | |||
} | |||
onScrollBeginDrag={handleOnScrollBeginDrag} | |||
onEndReached={handleLoadMore} | |||
onEndReachedThreshold={0.5} |
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.
updated this because the default was pretty choppy, scroll a small amount and get hard stop and loading indicator, was not very scientific but this seems to be smoother and load ahead of time when scrolling at a reasonable rate.
|
||
const ESTIMATED_ITEM_SIZE = 60 | ||
|
||
export function AboveTheFoldFlashList<ItemType>(props: AboveTheFoldFlashListProps<ItemType>) { |
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.
added a new component rather than replacing everywhere because I tried and it broke a bunch of stuff, they are probably just style issues
@@ -188,9 +208,9 @@ const AutosuggestResultsFlatList: React.FC<{ | |||
} | |||
|
|||
return ( | |||
<Flex> | |||
<Flex style={{ width: "100%", height: "100%" }}> |
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 flashlist wouldn't render without this, maybe can be addressed in AboveTheFoldFlashList component to not break other surfaces if we migrate
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 great!
One thought: I think we might need to change the name of the newly added component AboveTheFoldFlashList
since it is not exactly representative of what this component is doing
fair, I just copied the other |
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.
Thanks so much for improving this - @dblandin will be very happy
19351d2
…to brian/search-perf
This PR resolves PHIRE-1309
Description
Fairly minor change that replaces some lists in search with flashlist but does make nice improvements in list performance.
Before
before-low.mov
After
after-low.mov
Follow-ups
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.