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

Use <button> in EuiToggleButton #3008

Closed
wants to merge 2 commits into from
Closed

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Mar 8, 2020

Summary

Fixes #2982
Used button instead of input element

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@myasonik
Copy link
Contributor

myasonik commented Mar 9, 2020

Hey Anish! Thanks for getting this started!

@snide Do you think we can deprecate EuiToggle? Scouring GitHub, I couldn't find any uses of it other than EuiButtonToggle...

@myasonik myasonik changed the title Fixes #2982 switched to button Use <button> in EuiToggleButton Mar 9, 2020
@cchaos
Copy link
Contributor

cchaos commented Mar 9, 2020

@myasonik I'd rather not deprecate utilities unless we absolutely have to. EuiToggle is just a bare-bones utility. I don't think it's necessary to remove during this PR, but we can discuss in a different issue.

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

OK! Then @anishagg17, I think the changes done here should be done in EuiToggle and then EuiToggleButton mostly handles themeing and such.

I also just want to give you a heads up that because this PR will cause breaking changes, the EUI team will have to discuss the best way to roll this out still

src/components/button/button_toggle/button_toggle.tsx Outdated Show resolved Hide resolved
src/components/button/button_toggle/button_toggle.tsx Outdated Show resolved Hide resolved
src/components/button/button.tsx Outdated Show resolved Hide resolved
Comment on lines +50 to +51
labelOn?: string;
labelOff?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite understanding why we need label, labelOn, and labelOff.... The labels, based on the value of the toggle, should be handled by the consuming application and not this component itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's maybe a bit contrived... Not sure if it was the best way to solve this. (Very open to suggestions!) Here's the thing:

If we don't change the label of the text for the button, and only rely on style to communicate that the button is toggled on or off, we need to add aria-pressed="true"|"false".

If we do have separate texts for each state, then we shouldn't add aria-pressed at all.

@myasonik
Copy link
Contributor

@anishagg17 Thank you so much for starting this PR! Your work here sparked some great discussion internally.

After looking at our existing Button components and the aria requirements for toggle buttons, we've decided to deprecate both the utility and this component all together.

Soon, I'll be writing up some docs to add to EUI on the path forward and how to recreate toggle buttons with our existing components.

@myasonik myasonik closed this Mar 10, 2020
@anishagg17 anishagg17 deleted the toggle branch March 10, 2020 21:23
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.

Toggle button a11y
4 participants