feat(pagination): improve keyboard navigation and accessibility - #2700
feat(pagination): improve keyboard navigation and accessibility#2700SaiYugandhar03 wants to merge 3 commits into
Conversation
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
📝 WalkthroughWalkthroughPagination now uses explicit page and jump labels, ChangesPagination accessibility
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
98156f5 to
da856cc
Compare
da856cc to
95976a3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@packages/core/src/components/pagination/pagination.tsx`:
- Around line 203-210: Update the overflow button labels and handlers in the
pagination component so the announced page jump matches the actual selected-page
movement at boundaries; either compute each button’s target delta for its
aria-label or make both handlers consistently move three pages, while preserving
the existing pagination behavior and tests.
- Around line 168-171: Localize the accessible labels and announcements
generated by the pagination component, including the “Go to” page label, both
jump announcements, and the “Pagination” label, instead of concatenating
hard-coded English with i18nPage. Reuse the existing localization mechanism or
add localized inputs, and if public props are introduced, update their JSDoc,
`@since` metadata, Storybook stories, and test-app examples.
- Around line 168-171: Update the selected-page attribute construction in the
pagination button rendering to remove aria-selected, retaining
aria-current="page" for the current button. Adjust the pagination component test
assertions to verify aria-current="page" instead of aria-selected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f16b75a-61fe-4776-98a8-260eb781c6fb
📒 Files selected for processing (2)
packages/core/src/components/pagination/pagination.tsxpackages/core/src/components/pagination/test/pagination.ct.ts
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@packages/core/src/components/pagination/pagination.tsx`:
- Around line 168-169: Add a changeset for the affected pagination package
documenting the public accessibility behavior changes implemented around the
pagination component, including aria-current, accessible labels, navigation
semantics, and keyboard focus behavior.
- Line 188: Reuse the jump value declared in the pagination component’s jump
calculation in both backward and forward overflow handlers, replacing their
duplicate 2 * pageCount + 1 expressions. Ensure announced and selected
destinations use this shared jump value consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 043ffa93-26ec-4ab4-b11f-aa4d4f4ed33d
📒 Files selected for processing (2)
packages/core/src/components/pagination/pagination.tsxpackages/core/src/components/pagination/test/pagination.ct.ts
| 'aria-label': `${this.i18nPage} ${index + 1}`, | ||
| ...(this.selectedPage === index ? { 'aria-current': 'page' } : {}), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a changeset for the accessibility behavior change.
This PR changes the public ix-pagination accessibility contract, including aria-current, accessible labels, navigation semantics, and keyboard focus behavior. Add a changeset for the affected package, or explicitly justify why this change is internal-only.
As per path instructions, “Changesets are required for public API updates, behavior changes, styling/theming changes, accessibility changes, and bug fixes with user impact.”
🤖 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/core/src/components/pagination/pagination.tsx` around lines 168 -
169, Add a changeset for the affected pagination package documenting the public
accessibility behavior changes implemented around the pagination component,
including aria-current, accessible labels, navigation semantics, and keyboard
focus behavior.
Source: Path instructions
| let start = 0; | ||
| let end = Math.min(this.count, this.maxCountPages); | ||
| let pageCount = Math.floor((this.maxCountPages - 4) / 2); | ||
| const jump = Math.max(0, 2 * pageCount + 1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Reuse jump in both overflow handlers.
Line 188 defines jump, but the backward and forward handlers recompute the same expression at Lines 195 and 243. Use jump in both handlers so the announced destination and selected destination always use one calculation.
Suggested change
- this.selectPage(this.selectedPage - Math.max(0, 2 * pageCount + 1));
+ this.selectPage(this.selectedPage - jump);
...
- this.selectPage(this.selectedPage + Math.max(0, 2 * pageCount + 1));
+ this.selectPage(this.selectedPage + jump);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const jump = Math.max(0, 2 * pageCount + 1); | |
| this.selectPage(this.selectedPage - jump); | |
| ... | |
| this.selectPage(this.selectedPage + jump); |
🤖 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/core/src/components/pagination/pagination.tsx` at line 188, Reuse
the jump value declared in the pagination component’s jump calculation in both
backward and forward overflow handlers, replacing their duplicate 2 * pageCount
+ 1 expressions. Ensure announced and selected destinations use this shared jump
value consistently.
1 participant
💡 What is the current behavior?
The Previous (left arrow) button is not reachable via Tab, and screen readers incorrectly announce page buttons, ellipsis. This results in incorrect keyboard navigation and misleading accessibility announcements.
Jira ticket: IX-4345
🆕 What is the new behavior?
The Previous (left arrow) button is reachable via Tab, and screen readers correctly announce page buttons, ellipsis, and providing proper keyboard navigation and accurate accessibility announcements.
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
aria-current="page".