-
-
Notifications
You must be signed in to change notification settings - Fork 61
Implement virtual scrolling for org list #2819
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
Conversation
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
updates SubmenuController to use `LocalizeController` instead of `getComputedStyle` when determining RTL state, which saves an entire layout per call. on long lists, this adds up quickly.
container positioning
SuaYoo
left a comment
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 should implement org list backend pagination and filtering at some point, but agreed that this is a nice interim solution. Thanks!
Context
Because of the
getComputedStylecall on every shoelace menu item, rendering menu items for each item of a list is slow: each call togetComputedStyletriggers a reflow, and this happens for each menu item on each menu on each list item. There's some discussion of this here: shoelace-style/shoelace#773 (comment).Because of this, rendering the whole org list when there are hundreds of orgs locks up the main thread for sometimes several seconds. While pagination is possible for this, it seems a little pointless given we're already doing client-side filtering/search, so instead I've implemented virtualized scrolling.
Changes
This PR uses TanStack Virtual to implement virtualized scrolling for the superadmin org list. Rather than rendering all of the items all of the time, this only renders the items visible on screen, and updates when scrolling. This significantly improves the initial rendering time, as well as responsiveness when updating search and filters.
This does break
subgridrendering, which means we can't adjust the table spacing to match its contents automatically — but we've already got an overflow scroller on this table, so we can just set some sensible minimum column widths and amounts they expand when given space withminmax(<fixed_width>, <n>fr), and then handle long titles with truncation andtitleattributes.1It also adds a patch to
@shoelace-style/shoelacethat swaps outgetComputedStyleforLocalizeControllerto determine whether or not to display menu items as RTL. This significantly improves performance when rendering this list, as well as all other lists of items where each item has a dropdown menu. It also removes an older unused/unneeded patch for an older version of shoelace — shoelace now is now packaged slightly differently, and it seems the older patch isn't necessary anymore.Finally, it adds debounced updates when entering search values, which improves the responsiveness of the search input, since searching many items with Fuse.js can take a bit of time. In the future, maybe we want to look into PartyTown, which would let us run Fuse.js in a web worker separate from the main thread and therefore not block user input while searching at all, but for now this reduces how often searches cause user-input-blocking main thread hangs.
Testing
Log into an instance with many orgs as superadmin (I tested with prod, but be careful!), and poke around the superadmin org list. Try entering search terms, switching filters, and scrolling.
Ensure that:
Footnotes
We could use Shoelace tooltips for this, but given that a) native
titletooltips are likely much faster to render, and b) this is mostly internal-facing anyways, I thinktitleis the right approach here. ↩