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.

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

USWDS-Site - a11y: publish breadcrumb checklist #2853

Merged
merged 14 commits into from
Oct 16, 2024

Conversation

cathybaptista
Copy link
Contributor

@cathybaptista cathybaptista commented Oct 1, 2024

Summary

Added accessibility tests page for the breadcrumb component to inform users of accessibility test results.

Important

We need to update the changelog dates before merge.

Related issue

Closes #2854

Changelog

Preview link

Preview link:
Link to breadcrumb accessibility test page

Problem statement

Users need an clear and consistent way to check results of accessibility tests run on USWDS components.

Solution

This page provides user with accessibility test results run on the breadcrumb component in a consistent format.

Major changes

n/a

Testing and review

Follow these steps:

  • Confirm that both the main component page and the accessibility tests page have the correct compliance tag at the top.
  • Confirm that the main component page links to the accessibility tests page in the side navigation.
  • Confirm that the main component page links to the accessibility tests page from the accessibility guidance section.
  • Confirm that the main component page shows the accessibility tests summary.
  • Check that accessibility tests page provides the correct counts for each test status type.
  • Confirm the test checklist summaries and data are accurate.
  • Confirm no visual issues.
  • Confirm there is an appropriate changelog entry on both the main component page and the accessibility tests page.
  • 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 main).
  • Run npm run prettier:scss to format any Sass updates.
  • Run npm test and confirm that all tests pass.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.

@cathybaptista cathybaptista changed the title Cb publish breadcrumb checklist USWDS-Site - a11y: publish breadcrumb checklist Oct 1, 2024
@cathybaptista cathybaptista added Affects: Accessibility 🟡 Relates to the accessibility of our components A11Y Audit Findings Findings from the accessibility audit and removed Affects: Accessibility 🟡 Relates to the accessibility of our components A11Y Audit Findings Findings from the accessibility audit labels Oct 2, 2024
@cathybaptista cathybaptista marked this pull request as ready for review October 2, 2024 14:17
Copy link

@amycole501 amycole501 left a comment

Choose a reason for hiding this comment

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

I think the current wording is mostly OK however there isn't and won't be a current page link, per se. The wording for whatever the current page is needs to be focusable and announced as "current" by a screen reader. But it won't be underlined nor look like a link.

Secondly, the "learn more" link is broken.

Possible alternate text (but am open to suggestion)
Breadcrumb component may not be fully interactive with a keyboard. The wording for the current page in the breadcrumb list may not be focusable with a keyboard nor recognizable as the current page using a screen reader. You will need to test on your own implementation. More information: USWDS #2818.

We have logged this issue and are prioritizing it. Learn more about this issue on GitHub.

Copy link
Contributor

@finekatie finekatie left a comment

Choose a reason for hiding this comment

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

I thought I saw a typo in preview but I don't see it here in the code. All good except wondering what Amy or Alex think about the wording for one gherkin.

- summary: Breadcrumb links provide location within a set of pages.
summary_additional: |
When navigating through different pages,
the breadcrumb provides descriptive information of your location on the site.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @amycole501 or @alex-hull , does this read well to you? I feel like it's a little confusing when I scan it. If I'm off, just accept and I'll move on :)

Choose a reason for hiding this comment

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

It could be reworded to say "...the breadcrumb provides clarity on your location within the site."

Copy link
Contributor

@finekatie finekatie Oct 3, 2024

Choose a reason for hiding this comment

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

I like it @amycole501! Hey @cathybaptista who makes the change on this?

Copy link
Contributor Author

@cathybaptista cathybaptista Oct 9, 2024

Choose a reason for hiding this comment

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

Hi, @amycole501 and @finekatie! Apologies for the delay, these issues are fixed in b4c4c4b.

The only question I have is that I just want to verify that the "learn more" link is now going to the right place. :)

@finekatie finekatie self-requested a review October 3, 2024 19:55
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.

Looking good! I left a couple questions for a11y and request for a necessary test status change.

Testing checklist

  • Confirm that both the main component page and the accessibility tests page have the correct compliance tag at the top.
  • Confirm that the main component page links to the accessibility tests page in the side navigation.
  • Confirm that the main component page links to the accessibility tests page from the accessibility guidance section.
  • Confirm that the main component page shows the accessibility tests summary.
  • Check that accessibility tests page provides the correct counts for each test status type.
  • Confirm the test checklist summaries and data are accurate.
    • 3.2.3 consistent navigation should be conditional
    • 2.4.8 also conditional?
      • 2.4.8 is more of a site wide option. Breadcrumb is the solution to this
      • Still, it relies on the user implementing this correctly
    • 4.1.2 - Refers to current page listing as a link (which it currently isn’t). Also uses refers to it as “Furthers to the right” which we might want to rephrase to final list item or something along those lines.
  • Confirm no visual issues.
  • Confirm there is an appropriate changelog entry on both the main component page and the accessibility tests page.

_data/accessibility-tests/breadcrumb.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/breadcrumb.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/breadcrumb.yml Outdated Show resolved Hide resolved
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.

LGTM pending comments being resolved.

type: component
changelogURL:
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.

@@ -2,6 +2,11 @@ title: Breadcrumb
type: component
changelogURL:
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.

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.

All previous conflicts resolved, LGTM! Thanks @cathybaptista 👍

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.

Approving since comments have been resolved. Ready for FFR.

@thisisdano thisisdano merged commit fc5a598 into main Oct 16, 2024
11 checks passed
@thisisdano thisisdano deleted the cb-publish-breadcrumb-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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants