brand icon indicating copy to clipboard operation
brand copied to clipboard

Fix incorrect role and focus order in Pagination component

Open joshfarrant opened this issue 7 months ago • 3 comments

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 usePaginationPages hook to PaginationItem as 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 buildComponentData from model.ts and instead put that logic inside the PaginationItem component.
  • 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 removing tabindex from the ellipsis
  • as="span" has been removed from the ellipsis element. This was leaking through to the DOM due to as="span" not being a valid prop on our Link component. 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-label was 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-label when 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 snapshots label 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

joshfarrant avatar May 14 '25 16:05 joshfarrant

🦋 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

changeset-bot[bot] avatar May 14 '25 16:05 changeset-bot[bot]

🟢 No design token changes found

github-actions[bot] avatar May 14 '25 16:05 github-actions[bot]

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

github-actions[bot] avatar May 14 '25 16:05 github-actions[bot]

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

rezrah avatar Jun 04 '25 09:06 rezrah

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.

joshfarrant avatar Jun 04 '25 10:06 joshfarrant