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

Add accessible column in component statuses #2258

Merged
merged 15 commits into from Sep 27, 2022

Conversation

josepmartins
Copy link
Contributor

@josepmartins josepmartins commented Aug 23, 2022

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

References

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • 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.

@changeset-bot
Copy link

changeset-bot bot commented Aug 23, 2022

🦋 Changeset detected

Latest commit: 67c378b

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2022

size-limit report 📦

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

@josepmartins josepmartins changed the title Add new accessible frontmatter variable Add new accessible frontmatter variable Aug 23, 2022
@josepmartins josepmartins changed the title Add new accessible frontmatter variable Add new accessible frontmatter variable Aug 23, 2022
@josepmartins josepmartins temporarily deployed to github-pages August 23, 2022 15:43 Inactive
Copy link
Collaborator

@pksjce pksjce left a comment

Choose a reason for hiding this comment

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

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

@josepmartins
Copy link
Contributor Author

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 josepmartins marked this pull request as ready for review August 26, 2022 13:56
@josepmartins josepmartins requested review from a team and siddharthkp August 26, 2022 13:56
@josepmartins josepmartins added docs Documentation and removed status: wip labels Aug 26, 2022
@siddharthkp
Copy link
Contributor

siddharthkp commented Aug 26, 2022

(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

@josepmartins josepmartins temporarily deployed to github-pages August 26, 2022 14:05 Inactive
@josepmartins josepmartins temporarily deployed to github-pages August 29, 2022 08:01 Inactive
@josepmartins
Copy link
Contributor Author

josepmartins commented Aug 29, 2022

(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 josepmartins changed the title Add new accessible frontmatter variable Add a11yReviewed frontmatter variable Aug 29, 2022
@josepmartins josepmartins temporarily deployed to github-pages August 29, 2022 08:17 Inactive
@siddharthkp
Copy link
Contributor

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.

@josepmartins josepmartins temporarily deployed to github-pages September 12, 2022 14:29 Inactive
@josepmartins josepmartins temporarily deployed to github-pages September 12, 2022 14:53 Inactive
@josepmartins josepmartins force-pushed the josepmartins/accessible-frontmatter-variable branch from 3d3ea71 to 6dfd2cc Compare September 12, 2022 15:24
@josepmartins josepmartins temporarily deployed to github-pages September 12, 2022 15:30 Inactive
@josepmartins josepmartins temporarily deployed to github-pages September 13, 2022 13:33 Inactive
@josepmartins josepmartins temporarily deployed to github-pages September 19, 2022 09:03 Inactive
@josepmartins josepmartins temporarily deployed to github-pages September 21, 2022 09:42 Inactive
@josepmartins josepmartins force-pushed the josepmartins/accessible-frontmatter-variable branch from c2e4893 to a706668 Compare September 21, 2022 12:10
@josepmartins josepmartins changed the title Add a11yReviewed frontmatter variable Add accessible column in component statuses Sep 21, 2022
@josepmartins josepmartins temporarily deployed to github-pages September 21, 2022 12:22 Inactive
@josepmartins josepmartins force-pushed the josepmartins/accessible-frontmatter-variable branch from 7c26f0f to 6566a67 Compare September 21, 2022 12:25
@josepmartins josepmartins force-pushed the josepmartins/accessible-frontmatter-variable branch from 6566a67 to 57680fa Compare September 21, 2022 12:26
@josepmartins josepmartins temporarily deployed to github-pages September 21, 2022 12:34 Inactive
@josepmartins josepmartins self-assigned this Sep 21, 2022
@josepmartins josepmartins temporarily deployed to github-pages September 27, 2022 12:07 Inactive
Copy link
Contributor

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Looks good! ship it :)

@josepmartins josepmartins temporarily deployed to github-pages September 27, 2022 15:39 Inactive
@josepmartins josepmartins merged commit 270157a into main Sep 27, 2022
@josepmartins josepmartins deleted the josepmartins/accessible-frontmatter-variable branch September 27, 2022 16:04
@primer-css primer-css mentioned this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants