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

Toggle components #872

Merged
merged 19 commits into from
May 25, 2018
Merged

Toggle components #872

merged 19 commits into from
May 25, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented May 24, 2018

This PR creates three different components:

  1. EuiToggle: A generic toggling component that just overlays a checkbox (or radio) on top of the child supplied and listens for changes on this input.
  2. EuiButtonToggle: Since supplying a standard EuiButton to the EuiToggle component doesn't allow for pointer events on the actual button, this component adds some extra styles to simulate the same behavior but based on the pointer events on the input instead.

  1. EuiButtonGroup: A group of EuiButtonToggles that can either allow only one selection or multiple. It also has some extra specific styles as well.

Accessibility

Since each toggle is essentially just a checkbox (or radio), it requires a label which is added to the input via aria-label and this is what is read by the voiceover.

I also had to add tabIndex="-1" to the <button> element so that tabbing doesn't jump from input to button which doubles up on label reading.

Browser checks

  • Mac ,Chrome
  • Mac, FF
  • Mac, Safari
  • Windows 8, Edge

Fixes #293 #841

@chandlerprall
Copy link
Contributor

Curious why you overlayed the children with the input instead of wrapping it with a label tied to the hidden input (similar to https://codepen.io/CreativeJuiz/pen/BiHzp)?

That would keep the pointer events on the children themselves, and as a label's permitted content is phrasing, I think all of the elements we'd want to wrap are valid children.

@cchaos
Copy link
Contributor Author

cchaos commented May 24, 2018

I was following the convention of our EuiSwitch

@cchaos
Copy link
Contributor Author

cchaos commented May 24, 2018

And even further, wouldn't that require re-stying a whole new element (<label>) to look like a EuiButton? The current way allows us to use the EuiButton directly.

@snide
Copy link
Contributor

snide commented May 24, 2018

This is rad. Very quick notes on functionality (since it all looks great stylistically)

You probably want to provide some warning about accessibility on EuiToggle docs if you want to keep it that simple. I could see a lot of people copy / pasting that and forgetting to add focus states for the input. Don't know that we need to add a default state there (we could always focus ring), or just provide a callout that says... "Hey, don't forget to..."

Do you think we'll need the other size for button groups? I see you're forcing it to the small size, but flipping that value it still looks like it works ok. Should we make small the default, rather than a force? I'm mostly thinking about people putting these as filter toggles next to inputs. Don't feel strongly about it, guessing you prolly thought about that part of it a bit.

@cchaos
Copy link
Contributor Author

cchaos commented May 24, 2018

The EuiToggle is just supposed to be a helper and not really come with any styles, but I can understand the focus states. However, it'll get really hard to remove this focus ring for particular times they want custom focus states (like the EuiButtonToggle). I'll just add a callout and also point them to the EuiButtonToggle for most out-of-the-box use cases.

I thought about the sizing thing too, and yeah I can go ahead and change that to just be a default.

background-color: transparentize($color, .9);
}

&[class*="fill"] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this one with some comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

padding: 0 $euiSizeS;
}

.euiButton__text:empty {
Copy link
Contributor

Choose a reason for hiding this comment

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

Clever

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGreatTM!

@cchaos cchaos merged commit 846477f into elastic:master May 25, 2018
@cchaos cchaos deleted the toggle branch May 25, 2018 16:25
@cchaos cchaos mentioned this pull request May 29, 2018
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