Skip to content
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.

Sign up for GitHub

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

Jump to bottom

USWDS-Site: Added side navigation accessibility test page #2864

Merged
merged 9 commits into from
Oct 16, 2024
Merged

USWDS-Site: Added side navigation accessibility test page #2864

merged 9 commits into from
Oct 16, 2024

Conversation

RachelCorsino
Copy link
Contributor

@RachelCorsino RachelCorsino commented Oct 9, 2024 •
edited
Loading

Summary

Added accessibility test page for side navigation component

Related issue

Closes #2857

Preview link

Preview link: Side Navigation Component Page

Testing and review

Follow these steps:

  1. Confirm that both the main component page and the accessibility tests page have the correct compliance tag at the top.
  2. Confirm that the main component page links to the accessibility tests page in the side navigation.
  3. Confirm that the main component page links to the accessibility tests page from the accessibility guidance section.
  4. Confirm that the main component page shows the accessibility tests summary.
  5. Check that accessibility tests page provides the correct counts for each test status type.
  6. Confirm the test checklist summaries and data are accurate.
  7. Confirm no visual issues.
  8. Confirm there is an appropriate changelog entry on both the main component page and the accessibility tests page.

@RachelCorsino RachelCorsino marked this pull request as ready for review October 9, 2024 15:01
@mejiaj mejiaj changed the base branch from develop to main October 10, 2024 20:07
mejiaj
mejiaj requested changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@mejiaj mejiaj 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 creating this @RachelCorsino.

I've updated the base branch, made some formatting edits, and made two small suggestions.

_components/sidenav/accessibility-tests.md Outdated Show resolved Hide resolved
_components/sidenav/sidenav.md Outdated Show resolved Hide resolved
@mejiaj mejiaj linked an issue Oct 11, 2024 that may be closed by this pull request
CC a11y: publish side navigation checklist - create PR #2857
Closed
mahoneycm
mahoneycm requested changes Oct 15, 2024
View reviewed changes
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 good! One small change and one question for a11y

_components/sidenav/sidenav.md Show resolved Hide resolved
_data/accessibility-tests/side-navigation.yml Outdated
Comment on lines 42 to 47
- summary: Focus indicator is clearly visible in side navigation.
summary_additional: When you use a keyboard to tab into the side navigation, the links have a visible underline, bold style or other clear indication that links are interactive.
test_status: pass
test_type: keyboard
version_tested: 3.8.2
wcag_criterion: 2.4.7
Copy link
Contributor

Choose a reason for hiding this comment

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

question (non-blocking): Was there a reason we changed the Gherkin for focus indicators? This mentions a visible underline and bold style but neither is the default for focus indicators. Most of our other descriptions for 2.4.7 read:

Focus indicator is clearly visible. When you use a keyboard to navigate through the header, there will be a visible outline or other clear indication where the the focus is.

@amycole501 @alex-hull @finekatie

Copy link

Choose a reason for hiding this comment

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

Hmm...I'm not sure why that one got changed. I think it should reflect the way others have been written. The previous way potentially helps with some generalizing, versus tying people to those options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if this change is what you had in mind! 77b383e

mejiaj
mejiaj approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Code looks good and follows standards. Re-running CI checks because of a potential timeout.

_components/sidenav/sidenav.md Show resolved Hide resolved
_data/changelogs/component-side-navigation.yml Outdated
githubPr: 4163
githubRepo: uswds
versionUswds: 2.11.2
- date: NNNN-NN-NN
Copy link
Contributor

Choose a reason for hiding this comment

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

chore: Will need to add date before final merge.

_data/changelogs/component-side-navigation-accessibility.yml Outdated
title: Side navigation accessibility tests
type: component
items:
- date: NNNN-NN-NN
Copy link
Contributor

Choose a reason for hiding this comment

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

chore: Will need to add date before final merge.

@RachelCorsino
Copy link
Contributor Author

Also updated the test status for 2.4.8. Resolved in 8ac3f58

mahoneycm
mahoneycm approved these changes Oct 16, 2024
View reviewed changes
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!

@mejiaj mejiaj requested a review from alex-hull October 16, 2024 19:49
alex-hull
alex-hull approved these changes Oct 16, 2024
View reviewed changes
Copy link

@alex-hull alex-hull 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! Approved!

@thisisdano thisisdano merged commit 0446efb into main Oct 16, 2024
11 checks passed
@thisisdano thisisdano deleted the rc-side-nav-checklist branch October 16, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-hull alex-hull alex-hull approved these changes

@mahoneycm mahoneycm mahoneycm approved these changes

@thisisdano thisisdano thisisdano approved these changes

@mejiaj mejiaj mejiaj approved these changes

@cathybaptista cathybaptista Awaiting requested review from cathybaptista

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CC a11y: publish side navigation checklist - create PR
5 participants
@RachelCorsino @mejiaj @thisisdano @mahoneycm @alex-hull