fix: pagination keyboard controls#984
Conversation
🦋 Changeset detectedLatest commit: cf83ad1 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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Deploying solid-primitives with
|
| Latest commit: |
cf83ad1
|
| Status: | 🚫 Build failed. |
![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 @.storybook/ui/controls.tsx:
- Around line 1-8: Update the Button props’ onClick and onKeyUp types to use
Solid’s JSX.EventHandlerUnion for HTMLButtonElement with the corresponding event
types, or reuse JSX.ButtonHTMLAttributes, so tuple-form handlers passed from
pagination remain assignable.
In `@packages/pagination/src/index.ts`:
- Around line 171-183: Update goPage’s parent lookup to target the pagination
component directly, using the nearest nav ancestor or an equivalent
pagination-specific data attribute instead of the childElementCount < 3
heuristic. Keep focus selection scoped to that component so multiple paginations
and single-page configurations focus the interacted component’s aria-current
page.
In `@packages/pagination/test/index.test.tsx`:
- Around line 197-227: Make the “sets the focus after going to a new page” test
failure-safe by wrapping setup, assertions, and event dispatches in try/finally.
In the finally block, synchronously dispose the rendered root, remove the test
container from document.body, and restore HTMLElement.prototype.focus, ensuring
cleanup occurs even when an expectation throws.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eeb323cd-bd54-4160-b350-8323d451bf09
📒 Files selected for processing (6)
.changeset/bright-tigers-switch.md.storybook/ui/controls.tsxpackages/i18n/src/index.tspackages/pagination/src/index.tspackages/pagination/stories/pagination.stories.tsxpackages/pagination/test/index.test.tsx
|
@atk before I merge do you want to address the CR comments? I think the changes are fine aside from that. |
atk
force-pushed
the
fix/pagination-keyboard-controls
branch
from
c634cd4 to
cf83ad1
Compare
Done. There's still an unrelated test failing, but otherwise this is ready to merge. |
2 participants
When navigating the pagination with the keyboard, we could get into the situation where a button/link that previously had focus would be detached by the page switch, thus no longer having a parent reference to focus the current page. This is now fixed. Also, the stories broke whenever one switched the parameters. This is now rectified, too.
Addresses #286
Summary by CodeRabbit
Bug Fixes
Accessibility
Tests