Skip to content

Conversation

CTGM-Bixal
Copy link
Contributor

@CTGM-Bixal CTGM-Bixal commented Jul 2, 2024

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:

  1. Visit pagination preview page
  2. Ensure pagination "buttons" are showing up as links

Before opening this PR, make sure you’ve done whichever of these applies to you:

  • Confirm that this code follows the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
  • Run npm run prettier:sass to format any Sass updates.
  • Run npm test and confirm that all tests pass.

Copy link
Contributor

@cathybaptista cathybaptista left a 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. :)

Copy link
Contributor

@mahoneycm mahoneycm left a 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!

Copy link
Contributor

@mahoneycm mahoneycm left a 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!

@cathybaptista cathybaptista self-requested a review July 23, 2024 18:51
Copy link
Contributor

@amyleadem amyleadem left a 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?

@mahoneycm mahoneycm self-requested a review July 31, 2024 19:38
Copy link
Contributor

@mahoneycm mahoneycm left a 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.

@mejiaj
Copy link
Contributor

mejiaj commented Aug 20, 2024

@mahoneycm @amyleadem please move to FFR once both have approved.

@CTGM-Bixal CTGM-Bixal changed the title USWDS - Pagination: Fixed pagination border color contrast USWDS - Pagination: Updated text-decoration value to better show pagination links Aug 20, 2024
@CTGM-Bixal
Copy link
Contributor Author

PR description has been updated to account for text-decoration rather than color change.

Copy link
Contributor

@amyleadem amyleadem left a 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).

Copy link
Contributor

@amyleadem amyleadem left a 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.

@CTGM-Bixal CTGM-Bixal requested a review from amyleadem August 26, 2024 16:58
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @RachelCorsino

Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @RachelCorsino !

@amycole501 amycole501 added this to the 2024.09 September milestone Sep 9, 2024
@brunerae brunerae removed this from the 2024.09 September milestone Sep 26, 2024
Copy link
Contributor

@thisisdano thisisdano left a 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

@thisisdano thisisdano merged commit b8f37e7 into develop Oct 3, 2024
5 checks passed
@thisisdano thisisdano deleted the rc-update-pagination-border-color branch October 3, 2024 04:02
@thisisdano thisisdano mentioned this pull request Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS - Pagination: Borders around page number have poor color contrast
8 participants