-
Notifications
You must be signed in to change notification settings - Fork 67
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
Issue 698: Add a11y page #699
Conversation
@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. |
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! |
@zheyichn The Frontend PR Checks linter is failing due to a few |
@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.) |
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"> |
There was a problem hiding this comment.
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:
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:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
/accessibility-statement
,/vapt
.<table>
tags were used to ensure accessibility. Tested with VoiceOver on Mac - the web content is screen-readable./vapt
page were captured as the page is long).Closes #698