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 698: Add a11y page #699

Conversation

zheyichn
Copy link
Contributor

@zheyichn zheyichn commented Jun 19, 2024

  • Created two pages: /accessibility-statement, /vapt.
  • Plain html <table> tags were used to ensure accessibility. Tested with VoiceOver on Mac - the web content is screen-readable.
  • Examplary screenshots (Only partial content on /vapt page were captured as the page is long).
a11y-1 a11y-2 a11y-3

VAT-1-UPDATED

vpat-2

Closes #698

Copy link

vercel bot commented Jun 19, 2024

@zheyichn is attempting to deploy a commit to the Clean and Green Philly Team on Vercel.

A member of the Team first needs to authorize it.

@zheyichn zheyichn marked this pull request as draft June 19, 2024 20:34
@zheyichn
Copy link
Contributor Author

Hi! @CodeWritingCow @bacitracin, pls review when you get a minute. Thank you!

@zheyichn zheyichn marked this pull request as ready for review June 22, 2024 18:39
@CodeWritingCow CodeWritingCow changed the title DRAFT: add a11y page Add a11y page Jun 22, 2024
@CodeWritingCow CodeWritingCow changed the title Add a11y page Issue 698: Add a11y page Jun 22, 2024
@CodeWritingCow
Copy link
Collaborator

Hi! @CodeWritingCow @bacitracin, pls review when you get a minute. Thank you!

Hi @zheyichn let me review your PR within the next couple of days. I'm working this weekend, but I'll find time to review it. Thanks!

@CodeWritingCow
Copy link
Collaborator

@zheyichn The Frontend PR Checks linter is failing due to a few react/no-unescaped-entities errors. They should be pretty minor. Can you take a look?

Screen Shot 2024-06-22 at 4 38 30 PM

@zheyichn
Copy link
Contributor Author

@CodeWritingCow Thanks for catching this! It is fixed now. I will make sure to run lint check locally before creating PRs from now on.

@CodeWritingCow
Copy link
Collaborator

@CodeWritingCow Thanks for catching this! It is fixed now. I will make sure to run lint check locally before creating PRs from now on.

@zheyichn no worries! These linting errors are easy to miss. (I've missed them quite a few times too.)

@CodeWritingCow
Copy link
Collaborator

Hi @bacitracin when you get a chance, can you review this a11y PR? Thank you!

</tbody>
</table>

<h3 className="heading-xl font-semibold my-2">
Copy link
Collaborator

@bacitracin bacitracin Jun 23, 2024

Choose a reason for hiding this comment

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

I want to get your work merged so this isn't a blocker, but something we should either add in this PR or in a following one.

Right now when a screenreader user pulls up a list of all the tables on the page the tables don't have titles, so it looks like this:

IMG_7862_2

If you add an id to this h3 tag, say id="wcag-level-aa" and add an aria-describedby="wcag-level-aa" to the table tag on line 1032 then that will give the table a name and instead in the list it will look like:

IMG_7863

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bacitracin, thank you for the feedback! I have linked titles to table descriptions in the latest commit. Can you pls re-review?

The titles are now read and displayed when I navigate to specific tables, as shown in the screenshot below. However, table titles are still not displayed on the menu itself like your screenshot shows. Do you think this would be an issue?

table-titles

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zheyichn It is an issue because the shortcuts are how most screenreader users navigate the page, they usually don't read through line by line.

This is actually my bad, I meant aria-labelledby instead of aria-describedby. I was playing around with both and gave you the wrong one. Try that, and see if it will show up on the Voiceover tables page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bacitracin Got it, that makes sense! I have replaced them with aria-labelledby, and the titles are displayed in the menu now.
labelled-tables

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, thanks for updating! I'm going to go ahead and merge this to stage

Copy link
Collaborator

@bacitracin bacitracin left a comment

Choose a reason for hiding this comment

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

Amazing amazing work! It sounds great in the screenreader too. I have a comment about associating the tables with the table titles through the use of aria-describedby, but it's not a blocker. I will write up another ticket to update the VPAT.

@bacitracin bacitracin merged commit c634b6f into CodeForPhilly:staging Jun 24, 2024
1 of 2 checks passed
@CodeWritingCow CodeWritingCow added A11y Accessibility frontend labels Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A11y Accessibility frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants