fix(Table): move pagination outside refresh overlay so footer stays accessible#3371
Open
rzp-slash[bot] wants to merge 6 commits intomasterfrom
Open
fix(Table): move pagination outside refresh overlay so footer stays accessible#3371rzp-slash[bot] wants to merge 6 commits intomasterfrom
rzp-slash[bot] wants to merge 6 commits intomasterfrom
Conversation
…ccessible When isRefreshing is true, the RefreshWrapper overlay (position:absolute, height:100%) was covering the entire parent container including the pagination footer. This trapped users in an error/retry loop with no way to change the entries-per-page setting. Fix: wrap toolbar + table content in an inner relative BaseBox so the overlay only covers the table area. Pagination is rendered as a sibling outside that inner box and remains interactive at all times. - Add testID to refresh overlay Spinner for better testability - Add test: pagination controls accessible while isRefreshing=true - Add story: TableWithRefreshingAndPagination to verify fix visually - Delete stale snapshots (will be regenerated on next test run) Co-authored-by: kamleshchandnani <kamlesh.c@razorpay.com>
rzp-slash
Bot
requested review from
anuraghazra,
kamleshchandnani,
ravikumar-rajagopalan,
rohankokane-dev,
saurabhdaware,
swapnil-kr1 and
tewarig
as code owners
|
Contributor
|
✅ 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 5854b56:
|
…in test - Replace outer wrapper BaseBox with React fragment so pagination sits outside the refresh overlay without adding extra DOM elements. This avoids breaking any existing snapshots. - Restore original snapshot file from master (unchanged by this fix since no snapshot test uses the pagination prop) - Remove 'async' from test callback (no await used) - Remove unused 'queryByText' destructuring Co-authored-by: kamleshchandnani <kamlesh.c@razorpay.com>
…inner testID
- Fix TS2322: defaultPageSize={5} is not assignable to type '10 | 25 | 50 | undefined';
change to defaultPageSize={10}
- Update snapshots for 'should render table with isRefreshing' and
'should render table with TableEditableCell and Bordered cells' to
include data-testid attribute on the refresh overlay spinner
Co-authored-by: anay-rzp <anay.rajguru@razorpay.com>
…Refreshing test With nodes.slice(0, 10) and defaultPageSize=10, all items fit on a single page so the Next Page button is disabled and onPageChange never fires. Switch to full nodes array (25 items, 3 pages) so the button is active and the callback assertion passes. Co-authored-by: anay-rzp <anay.rajguru@razorpay.com>
Collaborator
🛡️ Coverage ReportSummaryFull Coverage Details |
…overlay The fragment approach failed visually because TableSurface is a flex column container. With pagination as a flex sibling of BaseBox, BaseBox's flex:1 could expand to fill the full TableSurface height - either pushing pagination outside overflow:hidden bounds or having RefreshWrapper (height:100% of expanded BaseBox) cover the pagination area visually. The correct fix: when pagination is present (hasPagination), render the table content inside a separate inner positioned container so that RefreshWrapper (position:absolute, height:100%) is definitively scoped to the inner box only. Pagination sits below the inner box, fully outside the overlay's covering area. When no pagination (the common case for snapshot tests), the original single-container DOM structure is preserved so no existing snapshots change. Co-authored-by: anay-rzp <anay.rajguru@razorpay.com>
…verlay
Two CSS root causes were preventing the fix from working visually:
1. Z-index stacking context: the inner BaseBox had position:relative but no
z-index (auto), so it did not create a stacking context. RefreshWrapper's
z-index:3 therefore participated in the outer stacking context and painted
above ALL later siblings — including the pagination that lives outside the
inner box.
Fix: add zIndex={0} to the inner BaseBox. This creates a stacking context
that contains the overlay's z-index, so it can no longer escape into the
pagination area.
2. height:100% resolution: percentage heights on absolutely-positioned elements
require the containing block to have an explicit height property. The inner
BaseBox gets its height from flex layout (flex:1), which some browsers do not
treat as 'definite' for percentage resolution, making the overlay height
unpredictable without an explicit height prop on Table.
Fix: replace width/height:100% with top/right/bottom/left:0 (inset-style
positioning). This pins all four edges to the containing block and works
correctly regardless of whether the containing block has an explicit height.
Co-authored-by: anay-rzp <anay.rajguru@razorpay.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When isRefreshing=true, the RefreshWrapper overlay (position: absolute, height: 100%) covers the entire parent BaseBox, including the TablePagination footer. This traps users: if an API call fails, the Retry option appears but they cannot change the entries-per-page selector because it is hidden under the overlay.
Fix
Introduced an inner BaseBox with position=relative that wraps only the toolbar + table content. The RefreshWrapper is scoped to this inner box. Pagination is rendered as a sibling outside the inner box and stays interactive at all times.
Before:
After:
Changes
Test plan
Generated with Claude Code