-
Notifications
You must be signed in to change notification settings - Fork 147
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(ActionList): add Virtualization #2471
Conversation
🦋 Changeset detectedLatest commit: c7b6a69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ PR title follows Conventional Commits specification. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c7b6a69:
|
Bundle Size ReportUpdated Components
|
Screen.Recording.2025-01-14.at.5.21.51.PM.movCheckboxes looks a bit flickery when scrolling 🤔 are they rerendering maybe? |
|
||
return ( | ||
<StyledListBoxWrapper | ||
isInBottomSheet={isInBottomSheet} |
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.
Let's try it out with a bottomsheet once in a real device. Ensure there is no weird issues
{itemCount < 30 ? ( | ||
childrenWithId | ||
) : ( | ||
<VirtualizedList |
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 ignore isVirtuzaliedList prop if itemCount is less than 30?
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.
yes
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.
Reduced it to 10 items
> [!NOTE] | ||
> | ||
> Current version only supports virtulization of fixed height list where items do not have descriptions. We'll be adding support for dynamic height lists in future versions |
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.
can we add this in docs as well ?
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 note in ActionList docs prop table. Will add better note when we write documentation for virtulization
{itemCount < 30 ? ( | ||
childrenWithId | ||
) : ( | ||
<VirtualizedList |
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.
maybe we can log an info that if item count is less switching to non virtualized list , might lead to a different experience in dev or prod env. if itemCount is less.
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.
Oh true good call. Will probably reduce the itemCount number and add a note in docs
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 note in ActionList docs props table. Will add better note when we write docs and tests and virtulization. Currently I have not added any documentation.
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.
might lead to a different experience in dev or prod env. if itemCount is less.
Let's wrap it in DEV flag, otherwise consumers may suddenly get lot of noise in their consoles.
They are rerendering (which is expected in virtulization). They shouldn't flicker when they render again though. I have created issue to track #2474. Will fix it separately |
…into actionlist/virtulization
Description
Adds
isVirtualized
prop to ActionList to virtualize the itemsExample: https://codesandbox.io/p/sandbox/trusting-feather-wphnnc?workspaceId=ws_CscdT6KxPm5VEoVzhtZAMq
Changes
Additional Information
Component Checklist