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

feat: Add a11y tests and GitHub action #460

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

ethanWallace
Copy link
Collaborator

@ethanWallace ethanWallace commented Jan 17, 2025

Summary | Résumé

Add cypress e2e test to test the accessibility of all pages on the documentation site. Also updated an old GitHub workflow to run these tests on pull request into main and merge into main.

Also includes some accessibility fixes that were identified by the tests, mostly misaligned HTML and headings.

How to use

Run tests in browser

  • Run npm run start to start a local version of the documentation site
  • Run npm run cy:open
  • This will open the cypress interface in the browser of your choice to view the tests

Run tests in terminal

  • Run npm run cy:run
  • This will start the site and run the tests. The Site will be closed after the tests complete.

cds-snc/design-gc-conception#1192

@ethanWallace ethanWallace marked this pull request as ready for review January 30, 2025 15:22
@ethanWallace ethanWallace requested review from melaniebmn and daine and removed request for melaniebmn January 30, 2025 15:22
@daine
Copy link
Collaborator

daine commented Feb 1, 2025

Looks good, the tests are all successful. However, I can't get the build to finish (that's what's failing on the amplify side).
It's not proceeding to this log on the build process:

2025-01-31T18:03:57.709Z [INFO]: # Completed phase: build
## Completed Frontend Build
2025-01-31T18:03:57.718Z [INFO]: ## Build completed successfully

the last message is

2025-01-31T18:03:57.670Z [INFO]: [11ty] Copied 752 files / Wrote 284 files in 8.94 seconds (31.5ms each, v2.0.1)

Locally, here's what I'm getting when I run build:

[11ty] Copied 754 files / Wrote 343 files in 754.77 seconds (2200.5ms each, v2.0.1)
2 pages found without an <html> element.
Pages without an outer <html> element will not be processed by default.
If adding this element is not possible, use the root selector config to target a different root element.

It seems like a pagefind error of some sort?

@ethanWallace
Copy link
Collaborator Author

[11ty] Copied 754 files / Wrote 343 files in 754.77 seconds (2200.5ms each, v2.0.1)
2 pages found without an element.
Pages without an outer element will not be processed by default.
If adding this element is not possible, use the root selector config to target a different root element.

I was seeing those messages too until I deleted _site and ran build again so I don't think that is causing issues on this PR.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-460.djtlis5vpn8jd.amplifyapp.com

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-460.d35vdwuoev573o.amplifyapp.com

@ethanWallace
Copy link
Collaborator Author

@daine I fixed the deployment issue.

I added a build:ci script to the package.json to run when someone runs test but since it started with build, it was also running when someone just ran the build script. That was preventing the deployment from moving onto the next step so that build:ci script has been renamed.

@@ -1,6 +1,6 @@
---
layout: 'layouts/base.njk'
github: https://github.com/cds-snc/gcds-components/tree/main/packages/web/src/components/gcds-radio
github: null
figma: https://www.figma.com/file/mh2maMG2NBtk41k1O1UGHV/GC-Design-System?type=design&node-id=462-110&mode=design&t=juCIOMIg2VKfCrQA-0
permalink: false
tags: ['radioEN', 'header']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove the "radio" pages? It's been deprecated for quite a while now.

@@ -1,6 +1,6 @@
---
layout: 'layouts/base.njk'
github: https://github.com/cds-snc/gcds-components/tree/main/packages/web/src/components/gcds-radio
github: null
figma: https://www.figma.com/file/o4SguSZdar2CCFzSkWNrmB/Syst%C3%A8me-de-design-GC?type=design&node-id=348-5024&mode=design&t=1DaL24vHpjRRfHHm-0
permalink: false
tags: ['radioFR', 'header']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment - should we remove these pages?

'log',
`${violations.length} accessibility violation${
violations.length === 1 ? '' : 's'
} ${violations.length === 1 ? 'was' : 'were'} detected`,
Copy link
Collaborator

@melaniebmn melaniebmn Feb 4, 2025

Choose a reason for hiding this comment

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

Can we add the impact level here as well, please?

melaniebmn

This comment was marked as outdated.

@melaniebmn melaniebmn self-requested a review February 4, 2025 23:47
Copy link
Collaborator

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

Left a few comments, looks great otherwise! I'm excited to get this merged in :)

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.

3 participants