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 - Add header accessibility tests page #2761

Merged
merged 19 commits into from
Aug 13, 2024

Conversation

RachelCorsino
Copy link
Contributor

@RachelCorsino RachelCorsino commented Jul 30, 2024

Summary

Added accessibility tests page for the header component.

Related issue

Closes #2739

Preview link

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 August 5, 2024 19: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 your work here @RachelCorsino. I've added some comments with change requests below. Please let me know if you have any questions!

_data/changelogs/component-header.yml Outdated Show resolved Hide resolved
_components/header/header.html Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that there is no alert on the accessibility section of the main page that points to the accessibility tests. Can you add it to this file like you did on the list component page? It should be possible by adding the following include:

{% include accessibility-tests/a11y-note.html %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 2aae0a9

Copy link
Contributor

@amyleadem amyleadem Aug 7, 2024

Choose a reason for hiding this comment

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

@RachelCorsino

Looks like we need to take a slightly different approach for this one since header.html is built with a different structure. Can you remove accessibility.md and instead add the include to header.html right below the "Header accessibility" element?

Edit: We might need to make some edits to a11y-note.html to make the link in the a11y note work. We can talk through the best options there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 6eece60

Let me know if I did this right!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @RachelCorsino!
That note typically belongs at the top of our other component accessibility guidance, so I took the liberty of moving the a11y-note to right below the "Header accessibility" header in ba8d7ba.

In that commit, I also updated the url in a11y-note.html so that it doesn't rely on componentKey from component.html. This should allow a11y-note.html to build the correct url. We should confirm that this doesn't have a negative effect on the a11y note links for existing pages like character count or accordion.

_components/header/header.html Show resolved Hide resolved
_data/accessibility-tests/header.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/header.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/header.yml Show resolved Hide resolved
_data/accessibility-tests/header.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/header.yml Show resolved Hide resolved
_data/accessibility-tests/header.yml Outdated Show resolved Hide resolved
@amyleadem
Copy link
Contributor

@RachelCorsino Also, can you update the PR title to match the format: "USWDS-Site - Add [component name] accessibility tests page"

@RachelCorsino RachelCorsino changed the title Rc publish header checklist USWDS-Site - Add header accessibility tests page Aug 6, 2024
RachelCorsino and others added 5 commits August 6, 2024 18:58
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
@RachelCorsino
Copy link
Contributor Author

@RachelCorsino Also, can you update the PR title to match the format: "USWDS-Site - Add [component name] accessibility tests page"

Title has been updated!

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 those updates @RachelCorsino. I've added some comments with some new discoveries and suggestions. Happy to work together on adding the test status table to the main component page - just let me know.

_data/changelogs/component-header.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/header.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/header.yml Show resolved Hide resolved
_data/accessibility-tests/header.yml Outdated Show resolved Hide resolved
_data/accessibility-tests/header.yml Show resolved Hide resolved
_data/accessibility-tests/header.yml Show resolved Hide resolved
_data/accessibility-tests/header.yml Show resolved Hide resolved
Copy link
Contributor

@amyleadem amyleadem Aug 7, 2024

Choose a reason for hiding this comment

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

@RachelCorsino

Looks like we need to take a slightly different approach for this one since header.html is built with a different structure. Can you remove accessibility.md and instead add the include to header.html right below the "Header accessibility" element?

Edit: We might need to make some edits to a11y-note.html to make the link in the a11y note work. We can talk through the best options there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note

Added additional assignments here because header.html does not inherit these assignments from component.html.

Copy link
Contributor

@amyleadem amyleadem Aug 12, 2024

Choose a reason for hiding this comment

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

Note

Updated the url assignment here because some pages (like header.html) do not inherit componentKey from component.html. We should confirm that this does not have a negative effect on existing pages like character count or accordion.

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! Thanks for your persistence on this one, @RachelCorsino

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.

Approved with gusto!

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.

One comment about some wording on a keyboard test. Otherwise, looks good to me!

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

@finekatie finekatie self-requested a review August 13, 2024 14:56
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.

Looks good to me!

Copy link
Member

@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.

Updating changelog dates

_data/changelogs/component-header.yml Outdated Show resolved Hide resolved
_data/changelogs/component-header-accessibility.yml Outdated Show resolved Hide resolved
@thisisdano thisisdano merged commit 3a13ceb into main Aug 13, 2024
11 checks passed
@thisisdano thisisdano deleted the rc-publish-header-checklist branch August 13, 2024 17:29
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.

CC a11y: publish header checklist - create PR
6 participants