Fix incorrect role and focus order in Pagination component
Summary
Fixed two reported bugs in the Pagination component, as well as an additional bug uncovered during the course of this work.
List of notable changes:
This may look like more of a refactor than it is. Check individual commits for more info, but the key changes are:
- Renamed the
usePaginationPageshook toPaginationItemas it was behaving more like a component than a hook; it accepted a props object and returned JSX - Unified the attribute calculation and component rendering logic by removing
buildComponentDatafrommodel.tsand instead put that logic inside thePaginationItemcomponent. - Updated stories to allow controls to be used across all stories
- While not necessary for this work, this change makes it easier for the component to be manually tested in Storybook.
- Added a load of new tests to better assert the expected behaviour of the component.
The majority of the key logic hasn't changed, it's just been moved around and simplified slightly.
Bonus HTML!
I also grabbed the rendered HTML from the Pagination component before and after these changes and dropped them into a couple of quick diffs so you can more easily see the changes to the rendered markup
Note the following changes:
- Previous & Next buttons both now have
role="button" tabindex="0"has been removed from all elements as it's not necessary since we're using the browser's default tab order. This also resolves https://github.com/github/primer/issues/3734 by removingtabindexfrom the ellipsisas="span"has been removed from the ellipsis element. This was leaking through to the DOM due toas="span"not being a valid prop on ourLinkcomponent. The previously described refactor and improved typing will prevent this category of issues from happening in the future.- When the Previous & Next buttons were disabled, their
aria-labelwas removed. This PR also resolves that bug.
What should reviewers focus on?
- Test the component and make sure it works as expected.
- I've added tests to cover this, but check all of the expected attributes are still in the right place.
Steps to test:
I've added controls to all Pagination stories, so poke around in Storybook to check everything.
Supporting resources (related issues, external links, etc):
- Closes https://github.com/github/primer/issues/3734
- Closes https://github.com/github/primer/issues/3735
- Fixes an additional bug discovered during the course of this work where the Previous/Next buttons would lose their
aria-labelwhen disabled.
Contributor checklist:
- [x] All new and existing CI checks pass
- [x] Tests prove that the feature works and covers both happy and unhappy paths
- [x] Any drop in coverage, breaking changes or regressions have been documented above
- [x] UI Changes contain new visual snapshots (generated by adding
update snapshotslabel to the PR) - [x] All developer debugging and non-functional logging has been removed
- [x] Related issues have been referenced in the PR description
Reviewer checklist:
- [ ] Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
- [ ] Check that tests prove the feature works and covers both happy and unhappy paths
- [ ] Check that there aren't other open Pull Requests for the same update/change
🦋 Changeset detected
Latest commit: 0ad4ad23f53c6aaacad0059fe108884bae7cbc67
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 7 packages
| Name | Type |
|---|---|
| @primer/react-brand | Patch |
| @primer/brand-docs | Patch |
| @primer/brand-primitives | Patch |
| @primer/brand-e2e | Patch |
| @primer/brand-fonts | Patch |
| @primer/brand-config | Patch |
| @primer/brand-storybook | Patch |
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
🟢 No design token changes found
🟢 No visual differences found
Our visual comparison tests did not find any differences in the UI.
I can see a change to the visual snapshot but wonder if that should have changed here? 🤔 Can't leave an inline comment so FYI @joshfarrant
Thanks @rezrah
That snapshot change is the result of this discussion where the default story was aligned with our docs example, so the snapshot change is expected.