-
Notifications
You must be signed in to change notification settings - Fork 52
Fix incorrect role and focus order in Pagination component #1009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d2397f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
case 'BREAK': { | ||
key = `page-${page.num}-break` | ||
content = '…' | ||
Object.assign(props, {as: 'span', role: 'presentation'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous implementation, this as
wasn't working as expected. The element was still rendered as an <a>
despite the as
prop here.
Note the rendered HTML:
This contributed (along with the tabIndex set here) to them still being focusable, and causing https://github.com/github/primer/issues/3734
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
1dc82c9
to
78f8e2c
Compare
78f8e2c
to
8f9bf5f
Compare
2cd62ed
to
c58a3d1
Compare
c58a3d1
to
dced700
Compare
dced700
to
31fe3e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Pagination
component to centralize page rendering logic into a new PaginationItem
component, fixes accessibility bugs around roles and focus behavior, and updates tests and stories to the StoryObj
format.
- Renamed and relocated pagination item logic, removing
buildComponentData
in favor of an inlinePaginationItem
- Fixed
role
,tabindex
, andaria-label
issues on ellipsis and prev/next buttons - Updated tests (added new assertions) and migrated stories to
StoryObj
API
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/react/src/Pagination/model.ts | Updated PageType definition and buildPaginationModel |
packages/react/src/Pagination/Pagination.tsx | Introduced PaginationItem component and removed old hook |
packages/react/src/Pagination/Pagination.test.tsx | Added new tests and fixed existing test logic |
packages/react/src/Pagination/Pagination.visual.spec.ts | Removed outdated default visual test |
packages/react/src/Pagination/Pagination.stories.tsx | Migrated to StoryObj format |
packages/react/src/Pagination/Pagination.features.stories.tsx | Migrated to StoryObj and updated meta inheritance |
.changeset/nine-cycles-pump.md | Documented accessibility fixes |
|
||
type PageType = { | ||
type: string | ||
export type PageType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PageType
definition is missing the precedesBreak
property, but buildPaginationModel
includes precedesBreak
on NUM pages. Add precedesBreak?: boolean
to PageType
.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
precedesBreak?: boolean |
Come on now , keep up
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:
usePaginationPages
hook toPaginationItem
as it was behaving more like a component than a hook; it accepted a props object and returned JSXbuildComponentData
frommodel.ts
and instead put that logic inside thePaginationItem
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:
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 removingtabindex
from 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 ourLink
component. The previously described refactor and improved typing will prevent this category of issues from happening in the future.aria-label
was removed. This PR also resolves that bug.What should reviewers focus on?
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):
aria-label
when disabled.Contributor checklist:
update snapshots
label to the PR)Reviewer checklist: