patternfly / patternfly-elements

PatternFly Elements. A set of community-created web components based on PatternFly design.
https://patternflyelements.org/
MIT License
375 stars 85 forks source link

fix(switch): implement internals controller #2702

Closed zeroedin closed 3 months ago

zeroedin commented 4 months ago

What I did

  1. Implementation of InternalsController
  2. Updates label API to a single <label> with children having the data-state

Testing Instructions

  1. View the deploy preview

Notes to Reviewers

1.

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 12a308fea6ef68887045317a73b3c7d3eb38f3bb

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

This PR includes changesets to release 1 package | Name | Type | | -------------------- | ----- | | @patternfly/elements | Major |

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

netlify[bot] commented 4 months ago

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit 0bfeb3b2909cb2621a23263f4901b998755451d2
Deploy Preview https://deploy-preview-2702--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

hellogreg commented 3 months ago

So, here's the current a11y state of the switch at patternflyelements.org (PFE) and in the deploy preview (DP). It's kind of a pick-your-poison with them.

TLDR:

patternflyelements.org

Without screen reader

Mac Safari/VoiceOver

Win Chrome/JAWS

Win Firefox/NVDA

iOS Safari/VoiceOver

Android Chrome/Talkback

Current deploy preview

Without screen reader

Mac Safari/VoiceOver

Win Chrome/JAWS

Win Firefox/NVDA

iOS Safari/VoiceOver

Android Chrome/Talkback

hellogreg commented 3 months ago

Also wanted to mention that the DP version is better than PFE in Mac/iOS Safari because of some updates @zeroedin made yesterday. If needed (as in, if we're not yet ready to go with this new version of switch), maybe those same fixes could be made to the live PFE version, too--fixing the label issue, the page scroll issue, and hiding the SVG from assistive tech.

bennypowers commented 3 months ago

@hellogreg did https://github.com/patternfly/patternfly-elements/pull/2702/commits/5dbdb14da9b3c10c46dabb5206c81b9e54738950 help?

hellogreg commented 3 months ago

Looks like it could be getting a little closer, but still has issues.

Still works fine in Safari/VO.

Chrome/Jaws:

Firefox/NVDA

zeroedin commented 3 months ago

This should get a major changeset because the label API changed, no? unless i'm mistaken and we already did that

You are correct we changed the label API by adding the <span> instead of multiple labels. This should get a major.

bennypowers commented 3 months ago

@nikkimk or @hellogreg if we could get a final sign off please