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

[Issue #347]: Add pa11y-ci checks #350

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

[Issue #347]: Add pa11y-ci checks #350

wants to merge 8 commits into from

Conversation

rylew1
Copy link
Contributor

@rylew1 rylew1 commented Jun 15, 2024

Ticket

Resolves #347

Changes

  • add pa11y-ci package
  • Add PR checks for mobile and desktop against the dynamically generated sitemap

context

  • Not sure if we want to document this in a decision record?

  • localhost:3000/sitemap.xml

image

@rylew1 rylew1 marked this pull request as draft June 15, 2024 02:00
Copy link

github-actions bot commented Jun 15, 2024

Coverage report for app

St.
Category Percentage Covered / Total
🟢 Statements 93.1% 81/87
🟢 Branches 82.35% 14/17
🟢 Functions 93.33% 14/15
🟢 Lines 93.59% 73/78

Test suite run success

16 tests passing in 5 suites.

Report generated by 🧪jest coverage report action from 3d2149c

@rylew1 rylew1 marked this pull request as ready for review June 15, 2024 02:23
@@ -11,7 +11,7 @@ import { defaultLocale, locales } from "./i18n/config";

// Don't run middleware on API routes or Next.js build output
export const config = {
matcher: ["/((?!api|_next|.*\\..*).*)"],
matcher: ["/((?!api|_next|sitemap|.*\\..*).*)"],
Copy link
Contributor Author

@rylew1 rylew1 Jun 15, 2024

Choose a reason for hiding this comment

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

Allows the atypical request to locahost:3000/sitemap.xml to resolve without being intercepted by Next middleware (so we can see this url in the browser)

Without this we get a 404 on the above url.

});

return appRoutes;
}
Copy link
Contributor Author

@rylew1 rylew1 Jun 15, 2024

Choose a reason for hiding this comment

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

Pull all routes (that are a page.tsx ) - the actual route "path" is in between [locale] a page.tsx

IE:
/
/health

This will also work for Next nested routes if there are any.

fi
done

echo -e "${GREEN}Server is ready after ~${WAIT_TIME} seconds.${NO_COLOR}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI script to wait for localhost:3000 - this kind of thing would be useful if you also needed to also wait for the API to load (if you have an e2e feature that you want to pa11y to test)

@rylew1 rylew1 requested a review from a team June 15, 2024 02:31
@rylew1 rylew1 added the ci/cd label Jun 15, 2024
@aligg
Copy link
Contributor

aligg commented Jun 18, 2024

If we don't add a decision record it could be nice to at least update the README to explain what pa11y-ci is, how to run it, what it's checking for, in brief.

Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

My understanding from talking to you about this is that accessibility checks aren't specific to tech stack. If that's true, I'd like to look into putting this in template-infra instead of template-application-nextjs so that we can have accessibility checks regardless of which application template they use

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: type out the full name ("accessibility" rather than "a11y")


jobs:
build:
name: Pa11y-ci tests
Copy link
Contributor

Choose a reason for hiding this comment

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

name the job based on what it's doing not the tool it's using e.g. "Accessibility" not "Pa11y"

@@ -0,0 +1,63 @@
name: Accessibility tests
Copy link
Contributor

Choose a reason for hiding this comment

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

follow the naming convention of other CI workflows and prefix with "CI" and use "Title Case"

image

@rylew1 rylew1 marked this pull request as draft June 21, 2024 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sitemap to Next.js (split ticket)
3 participants