-
Notifications
You must be signed in to change notification settings - Fork 1k
USWDS - Pagination: Updated text-decoration value to better show pagination links #5970
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
Conversation
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.
Hi, Rachel
I tested this, and your change does mean a color contrast of 5:1 versus 1:1 so I guess this passes? I'm not an alpha pro. Just eyeballing it, the change is much darker. I'll approve it if someone with a little more experience with contrast ratios can just verify that the new color contrast works. :)
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.
You're off to a good start here @RachelCorsino ! Left a change request with some additional context on the color adjust function and additional pitfalls to look out for.
Feel free to ping me if you want any additional help!
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.
Made a suggestion for a slightly more noticeable change though the suggestions still cause the the standard border to be quite a bit darker. I'm wondering if we want to improve the overall logic to maintain something closer to the current styles and offering a fallback instead.
It might be good to check in with the rest of the team to get a temperature check!
…ine to pagination border links
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.
Thanks for making the quick update, @RachelCorsino. I added a couple of suggestions below.
Also, can you update the PR title and description to reflect the change in solution approach?
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.
Looks great! This new approach is simple and effective. One request to update the PR description to match this approach and I'll give it the green ✅
issue (blocking): The PR description doesn't match this new approach.
suggestion: Update PR title, description, and testing instructions to match new text decoration approach.
@mahoneycm @amyleadem please move to FFR once both have approved. |
PR description has been updated to account for |
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.
Thanks @RachelCorsino.
I noticed there are two other text-decoration
style rules in usa-pagination.scss
that we should be able to safely remove. Can you check that it is safe to remove those style rules and if so, remove them?
Before approval, we'll also need to attach a changelog PR for this update. (Let me know if you need help creating that).
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.
Looks good to me!
One last update: Can you update the PR description to be a bit clearer? It might seem nitpicky but this description will be pulled into the release notes and will also be used for future reference as part of the component's history. Maybe something like this?
Added text underline styles to pagination links. Pagination links now have the same visual indicators as other USWDS text links. This should improve accessibility for the component.
Can you also update the problem statement since it doesn't quite capture the root issue? Maybe something like this:
Pagination links do not provide accessible link styles. Pagination borders around the page number have poor color contrast. The light gray ( #d1d1d1 ) on the white background in the example has a color contrast ratio of 1.5:1. We should provide a clear visual indication that the pagination list items are links."
Feel free to edit these suggestions for clarity.
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.
LGTM! Thanks @RachelCorsino
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.
LGTM! Thanks @RachelCorsino !
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.
A nice, simple, effective improvement
Summary
Added text underline styles to pagination links. Pagination links now have the same visual indicators as other USWDS text links. This should improve accessibility for the component.
Breaking change
This is not a breaking change.
Related issue
Closes #5939
Related pull requests
Changelog: Text-Decoration Style Fix for Pagination Component Accessibility #2793
Preview link
Pagination Component Page
Problem statement
Pagination links do not provide accessible link styles. Pagination borders around the page number have poor color contrast. The light gray ( #d1d1d1 ) on the white background in the example has a color contrast ratio of 1.5:1. We should provide a clear visual indication that the pagination list items are links.
Solution
Removed
text-decoration: none
so that the "buttons" are more clearly formatted as links.Testing and review
Follow these steps:
Before opening this PR, make sure you’ve done whichever of these applies to you:
git pull origin [base branch]
to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch isdevelop
).npm run prettier:sass
to format any Sass updates.npm test
and confirm that all tests pass.