-
Notifications
You must be signed in to change notification settings - Fork 626
SelectPanel: Add story to load more items on scroll #6633
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
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
@@ -765,6 +765,7 @@ function Panel({ | |||
onOpen={onOpen} | |||
onClose={onClose} | |||
overlayProps={{ | |||
id: id ? `${id}--dialog` : undefined, |
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.
Did you need to make this change to SelectPanel? Wondering if you could've passed an id
from your story in overlayProps
instead.
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.
You're right. Can do with just overlayProps
. Updated!
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.
Nice, thanks! Was wondering because if we wanted to go this route of loading more items on scroll, not having to modify SelectPanel here would mean we could do that today, without needing a primer/react update first.
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.
perfect! there is however another bug here :(
Scroll resets to top when the items change. Works well when primer_react_select_panel_remove_active_descendant
feature flag is on. I've asked what's the rollout plan on slack
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.
You should assume that FF to be on for this use case
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.
Pull Request Overview
This PR adds example stories to demonstrate SelectPanel performance with large datasets and implements a load-more-on-scroll feature. The changes explore rendering strategies for handling many items in SelectPanel components.
- Adds a dev story that renders 500 items at once for performance testing
- Implements an example story that initially loads 50 items and progressively loads more on scroll
- Includes performance measurement timing to compare rendering approaches
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx | Adds RenderMoreOnScroll story with scroll-based pagination, performance measurement, and user controls |
packages/react/src/SelectPanel/SelectPanel.dev.stories.tsx | Adds LotsOfItems story for testing performance with 500 items and timing measurement |
Note: This isn't a very good idea/implementation, i'm just exploring what's possible
primer_react_select_panel_remove_active_descendant
feature flag is on. Open story with feature flag on ↗