Skip to content

fix(Pagination): Use anchor instead of button for disabled prev/next controls - #4951

Merged
francinelucca merged 6 commits into
mainfrom
francinelucca/3395-prcpagination-best-practice-make-disabled-previousnext-control-non-focusable
Sep 16, 2024
Merged

fix(Pagination): Use anchor instead of button for disabled prev/next controls#4951
francinelucca merged 6 commits into
mainfrom
francinelucca/3395-prcpagination-best-practice-make-disabled-previousnext-control-non-focusable

Conversation

@francinelucca

@francineluccafrancinelucca commented Sep 11, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Closes #https://github.com/github/primer/issues/3395

Makes the Previous/Next controls in Pagination non-focusable by rendering them as as instead of button. Also adds role="link" to all 's in the component.

Changelog

New

  • Margin styles for disabled Next/Previous control to account for disparity between button borders and as

Changed

  • Render Next/Previous controls as a instead of button to make them non-focusable
  • Adds role="link" to all <a>'s (Next, Previous and page numbers) in the component to improve accessibility

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Integration Test PR: https://github.com/github/github/pull/341793

  • Go to preview Pagination Playground
  • Ensure pagination components looks and behaves exactly like production
  • Open DOM, confirm Next,Previous and all page controls are anchors with role="link" on them
  • Change current page to 1 (in playground controls)
  • Confirm "Previous" control is disabled, looks and behaves exactly like production
  • Open DOM, confirm disabled control is an aria-disabled anchor, not a button. Confirm there's no href or role attribute on it
    • Change current page to 15 (in playground controls)
  • Confirm "Next" control is disabled, looks and behaves exactly like production
  • Open DOM, confirm disabled control is an aria-disabled anchor, not a button. Confirm there's no href or role attribute on it

Merge checklist

@changeset-bot

changeset-botBot commented Sep 11, 2024•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: cca5a45

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactMinor

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

@francineluccafrancinelucca added the staff Author is a staff member label Sep 11, 2024
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4951 September 11, 2024 20:35 Inactive
@francineluccafrancinelucca changed the title fix(Pagination): a11y improvementsfix(Pagination): Use anchor instead of button for disabled prev/next controlsSep 11, 2024
@github-actions

github-actionsBot commented Sep 11, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js97.43 KB (-0.01% 🔽)
packages/react/dist/browser.umd.js97.73 KB (+0.01% 🔺)

@github-actions
github-actionsBottemporarily deployed to storybook-preview-4951 September 11, 2024 20:37 Inactive
francinelucca
francinelucca commented Sep 11, 2024
View reviewed changes
Comment threadpackages/react/src/Pagination/Pagination.tsx
Comment on lines +79 to +83
margin: 0 2px;

&:first-child {
margin-right: 6px;
}

@francineluccafrancineluccaSep 11, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do this to enforce no visual regressions when changing from button to anchor to account for the 2px border all buttons have. Couldn't find a mapping in the theme spaces but happy to do this differently if there's a better alternative!

@francinelucca
Merge branch 'main' into francinelucca/3395-prcpagination-best-practi…
06e46ec
…ce-make-disabled-previousnext-control-non-focusable
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4951 September 12, 2024 12:45 Inactive
@francinelucca
francinelucca marked this pull request as ready for review September 13, 2024 15:36
@francinelucca
francinelucca requested a review from a team as a code ownerSeptember 13, 2024 15:36
@francinelucca
Merge branch 'main' into francinelucca/3395-prcpagination-best-practi…
84ce8ca
…ce-make-disabled-previousnext-control-non-focusable
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4951 September 13, 2024 15:38 Inactive
TylerJDev
TylerJDev requested changes Sep 16, 2024
View reviewed changes

@TylerJDevTylerJDev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick comment, but otherwise looks good!

Comment threadpackages/react/src/Pagination/model.tsx Outdated
content = 'Previous'
if (page.disabled) {
Object.assign(props, {as: 'button', 'aria-disabled': 'true'})
Object.assign(props, {'aria-disabled': 'true'})

@TylerJDevTylerJDevSep 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add role="link" here, since I believe we'd only need it when the link is disabled. This should allow us to remove it below in the else condition

@francinelucca
Merge branch 'main' of github.com:primer/react into francinelucca/339…
311f98d
…5-prcpagination-best-practice-make-disabled-previousnext-control-non-focusable
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4951 September 16, 2024 15:14 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4951 September 16, 2024 15:20 Inactive
@primer-integration

primer-integrationBot commented Sep 16, 2024

Copy link
Copy Markdown

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/341793

@francinelucca
francinelucca added this pull request to the merge queueSep 16, 2024
Merged via the queue into main with commit c9009deSep 16, 2024
@francinelucca
francinelucca deleted the francinelucca/3395-prcpagination-best-practice-make-disabled-previousnext-control-non-focusable branch September 16, 2024 17:21
@primerprimerBot mentioned this pull request Sep 16, 2024
Merged
TylerJDev pushed a commit that referenced this pull request Sep 23, 2024
@francinelucca@TylerJDev
fix(Pagination): Use anchor instead of button for disabled prev/next …
1d18f0e
…controls (#4951)
* fix(Pagination): a11y improvements
* Create friendly-boats-serve.md
* fix(Pagination): only add role link to disabled anchors
@primerprimerBot mentioned this pull request Oct 18, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TylerJDevTylerJDevTylerJDev approved these changes

Assignees

No one assigned

Labels

staffAuthor is a staff memberstatus: review needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@francinelucca@TylerJDev