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

Website: Adds a11y audit in acceptance tests #1089

Merged
merged 26 commits into from
Jan 26, 2023
Merged

Conversation

MelSumner
Copy link
Contributor

@MelSumner MelSumner commented Jan 18, 2023

📌 Summary

If merged, this PR will add acceptance tests for each page so we can use ember-a11y-testing on our website.

  • adds an acceptance test for each page in the website
  • adds the a11y audit
  • fixes the issues in failing tests
  • cleans up some typos and linting failures that I came across

Related PRs:


💬 Please consider using conventional comments when reviewing this PR.

@MelSumner MelSumner requested a review from a team January 18, 2023 21:50
@vercel
Copy link

vercel bot commented Jan 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hds-components ✅ Ready (Inspect) Visit Preview Jan 26, 2023 at 7:27PM (UTC)
hds-flight-website ✅ Ready (Inspect) Visit Preview Jan 26, 2023 at 7:27PM (UTC)
hds-website ✅ Ready (Inspect) Visit Preview Jan 26, 2023 at 7:27PM (UTC)

@vercel vercel bot temporarily deployed to Preview – hds-flight-website January 19, 2023 01:31 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components January 19, 2023 01:31 Inactive
@alex-ju alex-ju added the docs-website Content updates to the documentation website label Jan 19, 2023
@vercel vercel bot temporarily deployed to Preview – hds-flight-website January 20, 2023 19:24 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components January 20, 2023 19:25 Inactive
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

This looks good overall! It's a shame we have to have separate files with a similar configuration, but once they're set up I don't see us tweaking them often, so not a burdain from a maintenance perspective.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

@MelSumner added a comment, that I think is a blocker (we need to take a decision as a team if we want to follow this approach for the documentation).

@@ -24,7 +23,7 @@ A few parameters were omitted for clarity.
Add the correct `@route/@models/@model/@query` parameter to each Breadcrumb Item.

```handlebars
<Hds::Breadcrumb>
<Hds::Breadcrumb aria-label="breadcrumbs with routing params">
Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from passing the tests, what is the benefit of doing this? I don't think we can have an expectation that all the code snippets (rendered as components), for every possible component, now and in the future, in every possible context, will pass all the accessibility criteria.
The downside of this is that when the consumers will copy this code snippet (or other code snippets in other pages for other components) they will copy code that may not be suitable for their context, or even incorrect.
I am totally OK to make the HDS components as much accessible as we can (eg. if the aria-label is required for the Breadcrumb let's add a property for that) but I want to have a discussion, as a team, and evaluate pros and cons, of making also all the code snippets for the HDS components in the Helios documentation also pass all the accessibility tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would ask you to consider this: We expect our examples to work, we expect our CSS and JavaScript to be correct, why would Accessibility be any different?

  1. If I am a user with a screen reader, and I visit this page, I am presented with a list of breadcrumbs, with no way to differentiate among them. So it becomes a non-equitable discoverability issue: while a user who can see the screen can quickly differentiate among examples and copy the example that is appropriate for them, the user with AT has to go through each example and remember which examples are there, then try to find the one that they want to copy.
  2. I do not think our consumers will be confused by the presence of different aria-labels. The presence of unique labels will reinforce the Success Criteria and provide supplementary learning through the examples.

I hope you will re-consider your point of view.

Copy link
Contributor

Choose a reason for hiding this comment

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

When consumers copy and paste the code snippet into their project, do we expect them to keep the aria-label, or is this something they would likely remove?

If it's something they'll remove, is there a way to attach the aria-label to the code snippet block (container) instead of directly in the component itself?

Copy link
Contributor Author

@MelSumner MelSumner Jan 25, 2023

Choose a reason for hiding this comment

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

@didoo In order to unblock this PR, I can commit to a follow-up PR that explores potential alternative approaches that achieve the same outcome.

Copy link
Contributor Author

@MelSumner MelSumner Jan 25, 2023

Choose a reason for hiding this comment

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

@heatherlarsen @KristinLBradley there's already an aria-label on the component itself (default is aria-label="breadcrumb") so that would take precedence (here on the docs website)

To answer the first question: any application that uses internationalized strings would change the content-

so aria-label="breadcrumb" would be edited to be aria-label={{t "breadcrumb"}}

But this goes for anything that has a string value like @text etc. Pretty much everything they copy and paste already has to be heavily edited, since they are superficial examples and not specific to a product. We put the args/props and values there to demonstrate how it is done, and they copy/paste and edit to match their own use case.

@MelSumner
Copy link
Contributor Author

@didoo @Dhaulagiri I've created https://hashicorp.atlassian.net/browse/HDS-1454 to track the additional exploration for reducing the amount of code the developer needs to change from a code sample. I believe this should be sufficient to unblock this PR so we can get it merged before launch, and as such I've requested a re-review.

@MelSumner MelSumner requested a review from didoo January 26, 2023 14:11
@didoo
Copy link
Contributor

didoo commented Jan 26, 2023

@MelSumner sorry closed by accident

@didoo didoo closed this Jan 26, 2023
@didoo didoo reopened this Jan 26, 2023
@vercel vercel bot temporarily deployed to Preview – hds-flight-website January 26, 2023 14:17 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components January 26, 2023 14:18 Inactive
@MelSumner
Copy link
Contributor Author

MelSumner commented Jan 26, 2023

After further discussion with @Dhaulagiri, I've removed the aria-label from the code samples and skipped the a11y tests for the breadcrumb page for the time being; we will revisit at a later date.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

added a minor comment and a possible idea.

all good! 👍

website/tests/acceptance/about-test.js Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – hds-flight-website January 26, 2023 19:25 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components January 26, 2023 19:25 Inactive
@MelSumner MelSumner merged commit 1cb6666 into main Jan 26, 2023
@MelSumner MelSumner deleted the melsumner/axe-testing branch January 26, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants