-
Notifications
You must be signed in to change notification settings - Fork 328
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
Update accessibility docs #2039
Conversation
7faf842
to
04d26d2
Compare
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.
self review
We run a [Lighthouse](https://developers.google.com/web/tools/lighthouse) job in our CI/CD, which generates a "score" for all pages in our **Kitchen Sink** example documentation. | ||
The configuration for Lighthouse can be found in the `.github/workflows/lighthouserc.json` file. | ||
## Known limitations and manual auditing | ||
|
||
For more information about configuring Lighthouse, see [the Lighthouse documentation](https://github.com/GoogleChrome/lighthouse-ci/blob/main/docs/configuration.md). | ||
For more information about Accessibility in general, see [](../../user_guide/accessibility.md). |
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.
Unless somebody says not to, I am going to create a follow-up PR to remove the Lighthouse checks and configuration.
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.
Fine by me
"text": "PyData Theme", | ||
"image_dark": "_static/logo-dark.svg", | ||
"alt_text": "PyData Theme home", |
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.
This example is misleading because our code now sets alt=""
if a theme adopter sets the logo["text"]
field. This is to avoid hearing the same text repeated twice by the screen reader, for example: "PyData Theme home PyData Theme".
I created what I think is a more realistic example based on the assumption that most sites will only put their logo in the top left and not the project text.
I guess I should add a note explaining that only one of text
or alt_text
should be specified; only rarely should both be specified together.
I'm thinking the need for this much nuance also suggests that we should simplify the config somehow.
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 guess I should add a note explaining that only one of text or alt_text should be specified; only rarely should both be specified together.
Agree, let's add a note about this.
I'm thinking the need for this much nuance also suggests that we should simplify the config somehow.
+1, do you have any ideas?
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 don't know if there really is a way to simplify it because users would encounter the same issue if they were writing HTML. But I guess this is a good example of why I personally dislike config files, because it's a bit hard to convey through the config object itself how its state will affect the state of the end application.
Anyway, I realized that all of this is covered in much greater detail in a separate page, Branding and logo. So in my last commit I slimmed down this section and linked to that page.
however, be aware that they do not follow accessibility best practices for | ||
native button components such as using the correct `ARIA attributes | ||
<https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role>`__. |
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 felt that this did not help the theme adopter because it did not make a recommendation nor was the info helpful enough for a non-accessibility expert to make a judgment call, so I updated it with a specific recommendation (to use sparingly) and a specific example (about how the space bar doesn't work on links that look like buttons).
Co-authored-by: Daniel McCloy <[email protected]>
Thanks for reviewing @drammock |
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.
Thank you @gabalafou this looks pretty good.
Left a couple of suggestions but this looks ready to merge IMO.
We run a [Lighthouse](https://developers.google.com/web/tools/lighthouse) job in our CI/CD, which generates a "score" for all pages in our **Kitchen Sink** example documentation. | ||
The configuration for Lighthouse can be found in the `.github/workflows/lighthouserc.json` file. | ||
## Known limitations and manual auditing | ||
|
||
For more information about configuring Lighthouse, see [the Lighthouse documentation](https://github.com/GoogleChrome/lighthouse-ci/blob/main/docs/configuration.md). | ||
For more information about Accessibility in general, see [](../../user_guide/accessibility.md). |
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.
Fine by me
"text": "PyData Theme", | ||
"image_dark": "_static/logo-dark.svg", | ||
"alt_text": "PyData Theme home", |
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 guess I should add a note explaining that only one of text or alt_text should be specified; only rarely should both be specified together.
Agree, let's add a note about this.
I'm thinking the need for this much nuance also suggests that we should simplify the config somehow.
+1, do you have any ideas?
and [Chrome](https://developers.google.com/web/tools/chrome-devtools/accessibility/reference), | ||
have accessibility tools built-in as part of their web developer tools. | ||
These tools can help to quickly identify accessibility issues and often include links to standards. | ||
#### Browser tools |
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.
Do we want to include polypane? IMO is a great tool, the downside is one needs a license.
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.
Yeah, I had the same thought on my first pass but didn't add it because the document was structured a bit differently and I wasn't sure if we should promote a paid product.
But given that we both find it useful, I agree with you that it could help some people to know about it, so I added a sentence about it.
Co-authored-by: Tania Allard <[email protected]>
@trallard, I think because I made changes I need a re-review |
This pull request brings the accessibility docs up to date.
Some of the changes in this pull request involve hard-wrapping the text in the docs files. I feel like this itself is an accessibility improvement as long lines are harder to read. It's also makes text diffing in git better.