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

Add pa11y scanner #3949

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Add pa11y scanner #3949

merged 2 commits into from
Jun 20, 2024

Conversation

beechnut
Copy link
Contributor

@beechnut beechnut commented Jun 19, 2024

Previously, we did accessibility scanning for every file in every push / build / PR, which took > 25 mins each run.

Now, we more carefully curate the files that need to be scanned, to limit the scan time.

This PR:

  • Adds a per-page pa11y scan, which lists pages that have changed or whose layouts have changed.
  • Adds a site-wide pa11y scan: if files that affect the whole site change, take a sample of files from across the site to check
  • Incidentally deletes an unused layout
  • Pins the glob package to 8, because 9 and higher breaks something with the SVG Sprite package.

Commentary:

This commit gets us to the "Make it work" stage, not yet addressing "Make it right" or "Make it fast".

The code also reads a little weird, and I'm not sure a conditional "eleventy.after" event is the right design for this. I think one more refactor here would be wise.

Closes #3875

Previously, we did accessibility scanning for every file in every
push / build / PR, which took > 25 mins each run.

Now, we more carefully curate the files that need to be scanned, to
limit the scan time.

This commit:

- Adds `npm run pa11y:list-files` command which runs a site build and
  outputs the files to be scanned for the current changeset
- Adds a per-page pa11y scan, which lists pages that have changed or
  whose layouts have changed
- Adds a site-wide pa11y scan: if files that affect the whole site
  change, take a sample of files from across the site to check
- Incidentally deletes an unused layout
- Moves _plugins/ to lib/ and ignores lib/ in the 11ty build
- Pins the glob package to 8, because 9 and higher breaks something
  with the SVG Sprite package

The code reads a little weird, and one more refactor could help.
@beechnut beechnut marked this pull request as ready for review June 19, 2024 02:05
@beechnut beechnut requested a review from a team as a code owner June 19, 2024 02:05
@caleywoods
Copy link
Contributor

I added some non blocking suggestions related to readability, this feels like a great start Matt!

lib/pa11y.js Outdated
}

function didSitewideFilesChange() {
const result = doesIntersect(changedFileSet(), sitewideFileSet())
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking suggestion here, I might rename result to filesChanged.

lib/pa11y.js Outdated
return filesToWrite
}

async function pa11yScan ({ dir, results }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking question/suggestion: results sounds like it's an array of pages/files/documents sent from eleventy.after, if that's correct then maybe renaming results to have a little more context could help readability? If so I think the same renaming should occur where results is passed to the pa11yScan function.

@beechnut beechnut merged commit 42f4f37 into replatform-main Jun 20, 2024
5 checks passed
@beechnut beechnut deleted the replatform/pa11y branch June 20, 2024 17:10
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.

2 participants