fix(data-table): make virtualized loader rows scroll with content#774
fix(data-table): make virtualized loader rows scroll with content#774paanSinghCoder wants to merge 1 commit intomainfrom
Conversation
Loader skeleton rows were pinned to the bottom via position: sticky; they should render in normal flow at the end of the list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes modify the data table loader component's styling and semantics. The loader container's CSS class has been renamed from Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/raystack/components/data-table/components/virtualized-content.tsx (2)
171-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
role="rowgroup"is orphaned outsiderole="table"— WAI-ARIA ownership violation
VirtualLoaderRows(rendered at line 370, after the</div>that closesrole="table"at line 369) adds a<div role="rowgroup">wrapper that is not a descendant of anytable,grid, ortreegridelement. The WAI-ARIA spec explicitly constrainsrowgroupto be contained bygrid,table, ortreegrid. Failing to satisfy this required-context relationship violates WCAG Success Criterion 1.3.1 and will be flagged by tools such as axe-core.Two options:
Option A (minimal) — drop the orphaned role, since the loader container is intentionally outside the ARIA table:
♿ Proposed fix – remove invalid role
- <div role='rowgroup' className={styles.loaderContainer}> + <div className={styles.loaderContainer}>Option B (semantically correct) — move
VirtualLoaderRowsinside<div role="table">, after thevirtualBodyGroup:♿ Proposed fix – place loader rows inside the ARIA table
</div> {/* virtualBodyGroup */} + {showLoaderRows && ( + <VirtualLoaderRows + columns={visibleColumns} + rowHeight={rowHeight} + count={loadingRowCount} + /> + )} </div> {/* role="table" */} - {showLoaderRows && ( - <VirtualLoaderRows - columns={visibleColumns} - rowHeight={rowHeight} - count={loadingRowCount} - /> - )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-table/components/virtualized-content.tsx` around lines 171 - 203, The wrapper div in VirtualLoaderRows uses role="rowgroup" but is rendered outside the element with role="table", causing an ARIA ownership violation; to fix, remove the invalid role attribute from the outer div in VirtualLoaderRows (the div with className={styles.loaderContainer}) so it no longer claims rowgroup semantics when outside the table, or alternatively move the VirtualLoaderRows JSX so it is rendered inside the element that has role="table" (after the virtualBodyGroup) if you want to preserve rowgroup semantics; update references to the div in virtualized-content.tsx (the loaderContainer div and its children that map over columns/rows) accordingly.
171-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
role="rowgroup"placed outsiderole="table"— invalid ARIA ownershipPer WAI-ARIA 1.2,
rowgroupmust be owned by atable,grid, ortreegrid.VirtualLoaderRowsis rendered after the closing tag of<div role="table">(line 373), placing therole="rowgroup"wrapper and itsrole="row"/role="cell"descendants in an invalid context that screen readers may ignore or report as orphaned.Two options:
Option A (minimal fix) — drop the role from the loader wrapper since it is intentionally outside the table:
♿ Proposed fix – remove invalid role
- <div role='rowgroup' className={styles.loaderContainer}> + <div className={styles.loaderContainer}>Option B (semantically correct) — move
VirtualLoaderRowsinside<div role="table">, after thevirtualBodyGroup:</div> {/* virtualBodyGroup */} + {showLoaderRows && ( + <VirtualLoaderRows + columns={visibleColumns} + rowHeight={rowHeight} + count={loadingRowCount} + /> + )} </div> {/* role="table" */} - {showLoaderRows && ( - <VirtualLoaderRows - columns={visibleColumns} - rowHeight={rowHeight} - count={loadingRowCount} - /> - )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-table/components/virtualized-content.tsx` around lines 171 - 203, The loader markup currently uses role="rowgroup" (and nested role="row"/"cell") in VirtualLoaderRows (the JSX returned in virtualized-content.tsx), which sits outside the <div role="table"> and creates invalid ARIA ownership; fix by either: A) minimal fix — remove the role="rowgroup" on the outer div (and if you prefer keep semantics outside the table, also remove role="row" and role="cell" from the child divs) so the loader has no table ARIA roles, or B) semantic fix — move the VirtualLoaderRows render so it is placed inside the element that uses role="table" (after virtualBodyGroup) so the existing role="rowgroup"/"row"/"cell" attributes are valid; update references to the component/function name (VirtualLoaderRows or the function that returns this JSX) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/raystack/components/data-table/components/virtualized-content.tsx`:
- Around line 171-203: The wrapper div in VirtualLoaderRows uses role="rowgroup"
but is rendered outside the element with role="table", causing an ARIA ownership
violation; to fix, remove the invalid role attribute from the outer div in
VirtualLoaderRows (the div with className={styles.loaderContainer}) so it no
longer claims rowgroup semantics when outside the table, or alternatively move
the VirtualLoaderRows JSX so it is rendered inside the element that has
role="table" (after the virtualBodyGroup) if you want to preserve rowgroup
semantics; update references to the div in virtualized-content.tsx (the
loaderContainer div and its children that map over columns/rows) accordingly.
- Around line 171-203: The loader markup currently uses role="rowgroup" (and
nested role="row"/"cell") in VirtualLoaderRows (the JSX returned in
virtualized-content.tsx), which sits outside the <div role="table"> and creates
invalid ARIA ownership; fix by either: A) minimal fix — remove the
role="rowgroup" on the outer div (and if you prefer keep semantics outside the
table, also remove role="row" and role="cell" from the child divs) so the loader
has no table ARIA roles, or B) semantic fix — move the VirtualLoaderRows render
so it is placed inside the element that uses role="table" (after
virtualBodyGroup) so the existing role="rowgroup"/"row"/"cell" attributes are
valid; update references to the component/function name (VirtualLoaderRows or
the function that returns this JSX) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e35734a2-9d41-426b-b9ff-bcfd63cb8c9b
📒 Files selected for processing (2)
packages/raystack/components/data-table/components/virtualized-content.tsxpackages/raystack/components/data-table/data-table.module.css
paanSinghCoder
deleted the
feature/cld-3048-fix-datatable-loadingrows-in-virtualizedcontent
branch
|
Replaced by #776 (same diff, renamed branch). |
1 participant
Loader skeleton rows in
DataTable.VirtualizedContentwere pinned to the bottom viaposition: sticky. They should scroll with the list and render at the end of the loaded rows.Test plan