-
Notifications
You must be signed in to change notification settings - Fork 841
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
Remove react-virtualized from ComboBox #3173
Remove react-virtualized from ComboBox #3173
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_3173/ |
This change was requested by @chandlerprall but anyone can review |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3173/ |
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.
A couple initial things:
-
Can we remove
react-virtualized
from our deps now? -
In the "Virtualized" EuiComboBox example, when I select an option the list is scrolled to the top. Previously, scroll position was maintained after making selections.
src/components/combo_box/combo_box_options_list/combo_box_options_list.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.
There's a regression in functionality for the Groups example: clicking on a group label in the dropdown closes the list; compare with the published docs where nothing happens when clicking a group label.
src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx
Outdated
Show resolved
Hide resolved
src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx
Outdated
Show resolved
Hide resolved
386b61e
to
3c0c49e
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_3173/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3173/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3173/ |
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.
Changes LGTM! Tested in the PR's docs build and couldn't hit a snag.
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.
Meant to hit the Approve button
Summary
Removing
react-virtualized
in favor ofreact-window
.EuiSelectable
andEuiComboBox
were the only two consumers ofreact-virtualized
. WithEuiSelectable
moving toreact-window
, this made sense to do to drop EUI's footprint.react-window
is smaller, faster, and has better accessibility so it's pretty much a win on all fronts.Did the smallest amount of changes to make this happen but they do handle things like scrolling and item rendering a bit differently so it required a bit of code.
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Checked in IE11- [ ] Props have proper autodocs- [ ] Added documentation examples- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] A changelog entry exists and is marked appropriately