react icon indicating copy to clipboard operation
react copied to clipboard

Add accessible column in component statuses

Open josepmartins opened this issue 2 years ago • 6 comments

  • [x] Add the a11yReviewed frontmatter variable to the Select and ActionMenu components
  • [x] Add Accessible column in component statuses

References

  • https://github.com/github/primer/issues/1212
  • https://github.com/primer/doctocat/pull/468
  • https://github.com/primer/primer.style/pull/326

Merge checklist

  • [ ] Added/updated tests
  • [ ] Added/updated documentation
  • [x] Tested in Chrome
  • [x] Tested in Firefox
  • [x] Tested in Safari
  • [ ] Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

josepmartins avatar Aug 23 '22 14:08 josepmartins

🦋 Changeset detected

Latest commit: 67c378b0557add5d3f60eecaf718f64e81e1b1e4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Aug 23 '22 14:08 changeset-bot[bot]

size-limit report 📦

Path Size
dist/browser.esm.js 77.1 KB (0%)
dist/browser.umd.js 77.71 KB (0%)

github-actions[bot] avatar Aug 23 '22 14:08 github-actions[bot]

Wouldn't alpha/beta status mean that it is accessible? I guess it could be accessible but not satisfy other alpha/beta criteria. 🤔

Hey @pksjce! 👋 that will be the ideal state in the future and eventually this signal/variable might become redundant, but at this point, there are only a couple of React components that have been reviewed by the accessibility team and have no major issues. The idea in the short-mid term is to give more visibility to those signals and components.

josepmartins avatar Aug 26 '22 10:08 josepmartins

(not a blocker, tiny duplication)

is there a way to expose this value from the component checklist? https://github.com/primer/react/blob/main/docs/content/ActionMenu.mdx?plain=1#L366 cc @colebemis

siddharthkp avatar Aug 26 '22 13:08 siddharthkp

(not a blocker, tiny duplication)

is there a way to expose this value from the component checklist? https://github.com/primer/react/blob/main/docs/content/ActionMenu.mdx?plain=1#L366 cc @colebemis

That's a good point, didn't know about that property in the CheckList component, which makes me think that it might be better to align the name - a11yReviewed instead of accessbile - for consistency and clarity.

Shouldn't the CheckList use the value that comes from the frontmatter instead of the other way around? @siddharthkp

josepmartins avatar Aug 29 '22 08:08 josepmartins

Shouldn't the CheckList use the value that comes from the frontmatter instead of the other way around? @siddharthkp

I'm okay with either direction. I think we wanted to use the checklist in a react component and didn't need to publish it outside of the docs when it was built.

(scope creep, feel free to ignore) I remember someone mentioned it would be nice to record a11yReviewedAt with a date instead of a boolean so that we can show if a review happened a long time ago and shouldn't be trusted.

siddharthkp avatar Aug 29 '22 08:08 siddharthkp