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

feat: add radio component #2985

Merged
merged 12 commits into from
Apr 24, 2020
Merged

feat: add radio component #2985

merged 12 commits into from
Apr 24, 2020

Conversation

marjonlynch
Copy link
Contributor

Add radio web component

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

@marjonlynch marjonlynch force-pushed the users/marjon/add-radio branch from 3b32d58 to e04221a Compare April 21, 2020 23:14
@marjonlynch marjonlynch force-pushed the users/marjon/add-radio branch from e04221a to 4214ba0 Compare April 22, 2020 17:10
Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

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

Similar to checkbox, I think our intent here is that this control behaves and is communicated to AT just as the native HTML radio buttons are input[type="radio"]. My primary concern is whether or not we have validated that is the case here. I've had feedback (linked in this PR as comments) on the W3C repo that label may not be valid for ARIA radios. Given that our goal here is parity with the platform, I think we should validate our assumption that this gives us parity from an AT standpoint. If we have parity with platform controls in terms of how these behave with AT, we're all good.

Two things to look for which are top of mind (not exhaustive):

  1. Focus behavior differs from the platform for ARIA spec (noted here: https://w3c.github.io/aria-practices/#radiobutton)
  2. Label behavior and validity may be different from HTML radios.

Again, the list is not exhaustive and my primary point is that we should validate this works as expected with assistive tech. My primary concerns here stem from feedback on existing implementations (react) where our aria radios do not have parity with the platform.

The last thing I will point out is that the W3C keyboard guidance calls out specific navigation behaviors for radios within a toolbar. What is our story for that? A possible solution: If we have parity with the platform in how these are communicated to AT, I would expect that implementation to match how you would do it with native platform controls. If we don't have parity with AT, I think we need to consider if we should map to the aria specification and the aria radio behaviors.

@marjonlynch marjonlynch force-pushed the users/marjon/add-radio branch 2 times, most recently from be3022a to 76331ae Compare April 23, 2020 17:03
@marjonlynch marjonlynch force-pushed the users/marjon/add-radio branch from 2681459 to 6988d53 Compare April 23, 2020 19:43
@marjonlynch marjonlynch force-pushed the users/marjon/add-radio branch from 6988d53 to cc07b6e Compare April 24, 2020 00:15
@marjonlynch marjonlynch force-pushed the users/marjon/add-radio branch from cc07b6e to 7998012 Compare April 24, 2020 00:54
@marjonlynch marjonlynch merged commit fffae9f into master Apr 24, 2020
@marjonlynch marjonlynch deleted the users/marjon/add-radio branch April 24, 2020 01:12
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.

5 participants