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

Address ToggleSwitch a11y feedback, take 2 #3433

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

camertron
Copy link
Collaborator

@camertron camertron commented Jun 20, 2023

This PR addresses the accessibility feedback for the ToggleSwitch component, outlined in this issue: https://github.com/github/primer/issues/1867. It is a re-submission of #3251 which had to be reverted, see #3429 for details.

Namely it:

  1. Removes role=switch from the button.
  2. Uses aria-pressed in favor of aria-checked.
  3. Makes the status label (on/off) clickable.

Screenshots

No visual changes

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.

* Address ToggleSwitch a11y feedback

* Remove unused import

* Add changeset

* Fix tests
@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2023

🦋 Changeset detected

Latest commit: 82471b2

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 Minor

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

@camertron camertron changed the title Address ToggleSwitch a11y feedback (#3251) Address ToggleSwitch a11y feedback, take 2 Jun 20, 2023
@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
dist/browser.esm.js 101 KB (-0.02% 🔽)
dist/browser.umd.js 101.56 KB (-0.02% 🔽)

@camertron
Copy link
Collaborator Author

camertron commented Jun 23, 2023

Dotcom integration PR: https://github.com/github/github/pull/277343

There seem to be no test failures, which seems weird to me? I thought it was reverted because it broke something in prod? cc @broccolinisoup

@broccolinisoup
Copy link
Member

@camertron First of all, it is great news that it is not failing 🎉 But I am confused too. I think I have a guess why it failed before and why it doesn't now but let me know your thoughts 🙌🏻

If we look at the logs that we made the reverting decision based on, we saw the error of 'Unable to find an accessible element with the role "switch"' and because ToggleSwitch PR introduces role changes (i.e. Removes role=switch from the button.), we thought there are probably misusages at dotcom and they are causing issues. What are your thoughts??

I wonder if the ToggleSwtich PR failed because of other changes in the changeset impacted it?? Maybe the ones that we reverted, or the ones made through? 🤔 What do you think?

We still haven't managed to merge the upgrade PR due to all of these unrelated failings, so we can't really test it in the review lab 😥 I think the results would probably be more accurate if we can test it on the new version of PRC (after merging that PR)

@camertron
Copy link
Collaborator Author

Hey @broccolinisoup, sorry for the confusion, I made a mistake and somehow didn't bump primer/react correctly 😳 I just re-bumped it to the pre-release version and now I see test failures. Apologies! I'll get things fixed 👍

@broccolinisoup
Copy link
Member

@camertron No worries, all good!! Good luck 🤞🏻

Copy link
Member

@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.

Very nice!

Checked usage for component and looks safe to ship. :shipit:

@siddharthkp siddharthkp added this pull request to the merge queue Jul 3, 2023
Merged via the queue into main with commit cd086ae Jul 3, 2023
@siddharthkp siddharthkp deleted the toggle_switch_a11y_take2 branch July 3, 2023 09:34
@primer-css primer-css mentioned this pull request Jul 3, 2023
@broccolinisoup
Copy link
Member

Hi @siddharthkp! I think this PR was breaking some tests at dotcom - reference and we reverted it in the previous release. I don't think the failures are fixed yet? I don't see any new commits in the PR 🤔

siddharthkp added a commit that referenced this pull request Jul 4, 2023
@siddharthkp
Copy link
Member

siddharthkp commented Jul 4, 2023

I don't think the failures are fixed yet?

My bad! I misread your comment above and thought now the issues are fixed 😓

I will revert the revert 😭 and copy the description from this PR

Update: #3483

github-merge-queue bot pushed a commit that referenced this pull request Jul 4, 2023
* Revert "Address ToggleSwitch a11y feedback (#3251) (#3433)"

This reverts commit cd086ae.

* Update generated/components.json

---------

Co-authored-by: siddharthkp <siddharthkp@users.noreply.github.com>
@camertron camertron mentioned this pull request Jul 10, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants