-
Notifications
You must be signed in to change notification settings - Fork 192
Virtualized list with @tanstack/react-virtual #1964
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 573c5fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
e5b8141
to
8228a8e
Compare
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 tested and it really improves performance, well done 👏
Just a few questions / comments!
onChange, | ||
onClear, | ||
onCreateOption, | ||
virtualized = false, |
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.
Question here: I get that we want to keep the default behaviour as much as possible but wouldn't it be also possible to turn on the virtualization automatically if the number of items exceed a limit (for example for a dynamic list we don't know how many elements it could have)?
@@ -0,0 +1,6 @@ | |||
--- | |||
'@strapi/design-system': major |
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.
Since it's not really breaking anything, is it really necessary to bump to a major?
'@strapi/ui-primitives': major | ||
--- | ||
|
||
Add virtualization as an option to combobox list |
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.
We decided to also stick to conventional commits for changesets messages format :)
d83aa0f
to
3f031d4
Compare
There are some issue with the combobox input events (change, clear, blur) because of the virtualization. I'll post pone the fix on the combobox performance. |
3f031d4
to
75e38fd
Compare
75e38fd
to
573c5fa
Compare
|
The issue has been fixed by disabling the virtualization as soon as the list is being filtered. It solves most of the issues. |
What does it do?
Add optional virtualized list for combobox (implemented to the timepicker).
Why is it needed?
To improve the TimePicker (and Combobox) performances on long list.
How to test it?
Related issue(s)/PR(s)
Let us know if this is related to any issue/pull request
Before fix: https://www.loom.com/share/07fc90f6b02543b48e18a3f036150c70
After fix: https://www.loom.com/share/f740d3fd31424e6285ea05517bebb342