fix(listbox): unexpected scrollShadow on virtualized listbox #4784
Conversation
🦋 Changeset detectedLatest commit: ae70bfc The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 Git ↗︎
|
WalkthroughThis pull request patches two dependencies and refines the scroll behavior in virtualized listboxes. It introduces a new variable to capture the total virtual scroll height and adds corresponding data attributes to the listbox component. In parallel, the overflow detection hook is updated to reference these new attributes and to employ event listeners in the capture phase. The modifications are implemented to resolve the scroll shadow issue described in issue #4553. Changes
Sequence Diagram(s)sequenceDiagram
participant Listbox as VirtualizedListbox
participant Hook as useDataScrollOverflow
participant Browser as Scroll Event
Browser->>Listbox: Trigger scroll event
Listbox->>Listbox: Update data-virtual-scroll-top & data-virtual-scroll-height
Listbox->>Hook: Invoke overflow check with updated scroll data
Hook->>Hook: Calculate overflow states based on new scroll attributes
Assessment against linked issues
Possibly related issues
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/hooks/use-data-scroll-overflow/src/index.ts (1)
115-124: LGTM! Consider adding error handling for invalid attribute values.The changes correctly handle both virtualized and non-virtualized scenarios by using data attributes when available and falling back to element properties.
Consider adding error handling for invalid attribute values:
- const scrollHeight = +( - listbox?.getAttribute("data-virtual-scroll-height") ?? el.scrollHeight - ); + const virtualHeight = listbox?.getAttribute("data-virtual-scroll-height"); + const scrollHeight = virtualHeight ? Number(virtualHeight) || el.scrollHeight : el.scrollHeight; - const scrollTop = +(listbox?.getAttribute("data-virtual-scroll-top") ?? el.scrollTop); + const virtualTop = listbox?.getAttribute("data-virtual-scroll-top"); + const scrollTop = virtualTop ? Number(virtualTop) || el.scrollTop : el.scrollTop;packages/components/listbox/src/virtualized-listbox.tsx (2)
108-108: Consider memoizing virtualScrollHeight.The value should be memoized to prevent unnecessary recalculations during renders.
- const virtualScrollHeight = rowVirtualizer.getTotalSize(); + const virtualScrollHeight = useMemo(() => rowVirtualizer.getTotalSize(), [rowVirtualizer]);
176-180: Handle potential null values in data attributes.The current implementation might expose undefined values in data attributes.
{...getListProps()} - data-virtual-scroll-height={virtualScrollHeight} - data-virtual-scroll-top={parentRef?.current?.scrollTop} + data-virtual-scroll-height={virtualScrollHeight || 0} + data-virtual-scroll-top={parentRef?.current?.scrollTop || 0}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/ninety-flowers-teach.md(1 hunks)packages/components/listbox/src/virtualized-listbox.tsx(3 hunks)packages/hooks/use-data-scroll-overflow/src/index.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (3)
packages/hooks/use-data-scroll-overflow/src/index.ts (2)
127-131: LGTM! Accurate overflow detection for virtualized listbox.The changes correctly utilize the new scroll metrics to determine the overflow state, ensuring accurate detection in both virtualized and non-virtualized scenarios.
145-145: LGTM! Event capture phase ensures reliable scroll detection.Using the capture phase for scroll event handling ensures reliable detection regardless of the actual scroll target in virtualized scenarios.
Also applies to: 164-164
.changeset/ninety-flowers-teach.md (1)
1-7: LGTM! Clear and concise changeset.The changeset correctly identifies the affected packages and links to the relevant issue.
📝 Description
virtualization will be enabled when the number of items is greater than 50, which comes with scrollShadow. However, currently there is no way to disable it even passing
scrollShadowProps={{isEnabled: false}}.The reason is that the following data attributes were added to virtualized listbox instead of being added in
useScrollShadow.Checked with Roger, the reason is that in the virtualized-listbox, because of how tanstack virtualizer implementation, it makes that the scrolling component differs from the actual component that render the rows (the one that have very big number of height). Hence the useScrollShadow is not able to "detect" the scrolling component and update the data-overflow accordingly.
This PR is to
addEventListener(currently this line does nothing since it is not capturing anything)⛳️ Current behavior (updates)
With the following code, scroll shadow is visible even though setting
isEnabledtofalse.🚀 New behavior
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit