primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.15k stars 535 forks source link

📒[Docs] Accessibility Docs #513

Closed emplums closed 3 years ago

emplums commented 5 years ago
bdougie commented 5 years ago

@emplums have you started work on this yet?

I ask because I have been using @primer/components my side project and have started adding a11y specific tests for my React components. I am no expert on accessibility, which is why I am leveraging the following tests in my components. This helps catch most things and forces me to think about accessibility before merging changes to my components.

I actually wandered here because I found all my primer components were failing my a11y tests.

Full test: https://github.com/bdougie/open-sauced/blob/master/src/tests/Login.test.js

import {axe, toHaveNoViolations} from "jest-axe";
expect.extend(toHaveNoViolations);

test("container component should have no violations", async () => {
  const {container} = render(<Login />);
  const results = await axe(container);
  expect(results).toHaveNoViolations();

  cleanup();
});

I would be happy to share my findings and recommendation wherever these guides live.

emplums commented 4 years ago

@bdougie sorry for the late reply, in notification hell and just getting caught up! 😂 We haven't been running axe-core in specific component tests but have been using a11y linters that fail our builds if there are any a11y issues. Would definitely be interested in hearing more about what you were seeing especially if there are a11y changes that need to be made to the components themselves. Are the failing tests related to how primer components are implemented or how they are consumed?

As far as documentation on a11y, I think it would be helpful to have docs on how to use axe-core in consuming applications, etc though not sure if that's something that should live in Primer Components or general design/site building guidelines. Either way, if you have notes or something you'd like to share feel free to link them here so I can add them & credit you when we do figure out where guides like that should live!

bdougie commented 4 years ago

Hey @emplums, I also got into a bit of a notification storm after the holidays but now caught up.

To provide more context I added Primer Components pretty early in its infancy -- I think we originally talked about it in the GitHub lunchroom -- as part of my side project's design refresh.

I only recently added a11y tests after running through Kent's testing course and discovered you can test a11y.

Most of the a11y issues I discovered were note having an alt on the Avatar or Octicon components. Specific Example.

I have been writing on dev.to lately about my project and happy to work through a tips post on using a11y features Primer Components there first. Looking at the code now, I see I could has used the aria-label prop and that would be a useful tip to start with. I will share the post when I post it.


Related to the above: Does this code seem redundant?

<Octicon alt="star" verticalAlign="middle" icon={getIconByName("star")} />

What if the alt tag was added was added dynamically without the user needing to state that the Octicon is a "star" in two places?

// using the `getIconByName` function could implicitly add the alt tag will be added.

<Octicon verticalAlign="middle" icon={getIconByName("star")} />

Happy to kick this out into a new issue in primer/octicons for discussion, if you would like me to explore scoping this out.

emplums commented 4 years ago

Hey @bdougie! I'm working with @yaili this week to add a new section to each component's docs for a11y tips! Would love to see that article once it's done!

Regarding the redundant code above - in the case of the Octicons - most of the time we don't actually want to add alt text to an icon unless the icon is conveying something that isn't also conveyed with text nearby it (but ideally there would always be accompanying text). For instance, if there's a star icon and next to it we have the text Star we actually want the icon to be hidden from screen readers so that the screen reader doesn't read out star twice (once for the alt text on the icon and once for the actual text). For that reason I've decided not to automate the alt text in Octicons since the usefulness of alt text is pretty contextual and varies from case to case. This all should be documented better though! I'm working this week on augmented our docs a bit more for these kinds of things :)

To fix the axe-core test in this case the recommended action is to leave the alt text with a null value by doing alt="", empty values for the alt attribute will signal to the screen reader that the element can be skipped.

bdougie commented 4 years ago

The blog has been posted. Primer is mentioned in the last section as a false negative for automating my a11y testing.

https://dev.to/bdougieyo/accessibility-testing-in-react-with-jest-axe-l7k

yaili commented 4 years ago

@emplums happy to chat any time, also wanted to prioritise this for primer.style and/or design. ✨

bolonio commented 3 years ago

Hi, about the testing discussion here, I've read from @emplums that you haven't used axe-core tests yet. I don't know what the status so far. I've wrote a couple of articles that can be useful for you guys (not only for axe). Let me know what you think :)

https://www.adrianbolonio.com/en/accessibility-with-storybook/

https://www.adrianbolonio.com/en/accessibility-github-actions/

Have a nice day :) Greetings from Vienna (Austria)

bolonio commented 3 years ago

Hi, I've just created a new PR to increase the accessibility to the primer components, let me know what you think and we can discuss about it :)

https://github.com/primer/components/pull/1126

github-actions[bot] commented 3 years ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.