Add new d2l-button-toggle component. #5048
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| } | ||
|
|
||
| async _handleClick(pressed) { | ||
| this.pressed = pressed; |
There was a problem hiding this comment.
Shouldn't the underlying <button>s get aria-pressed="true"
There was a problem hiding this comment.
Not necessarily. Jeff and I did a bunch of investigation into that. Using aria-pressed is one approach but the guidelines (specifically the Mozilla description for aria-pressed) suggest that it's not required if the state is communicated via the label. Jeff preferred not using it due the confusion around the label and the state.
There was a problem hiding this comment.
Interesting, cool. Probably worth calling that out as a "strong recommendation" or requirement in the docs.
There was a problem hiding this comment.
Yeah agreed. I have a side conv with Jeff to craft something for this, since we'll want to provide some guidance around how they should set their labels.
| } | ||
|
|
||
| async _handleClick(pressed) { | ||
| this.pressed = pressed; |
There was a problem hiding this comment.
Interesting, cool. Probably worth calling that out as a "strong recommendation" or requirement in the docs.
| elem.focus(); | ||
| } | ||
|
|
||
| async _handleClick(pressed) { |
There was a problem hiding this comment.
Is there a use case for firing an event when the state changes? I'm worried that if we don't, consumers might wire up to the individual subtle/icon buttons and need to do some extra work to determine the state.
There was a problem hiding this comment.
Yeah, I was just mulling that over. I think you're right, and we should probably expose a specific event to avoid any confusion with them wiring up different handlers to each button.
| @@ -39,6 +39,7 @@ <h2 class="d2l-heading-3">Components</h2> | |||
| <li><a href="components/button/demo/button-add.html">d2l-button-add</a></li> | |||
| <li><a href="components/button/demo/button-icon.html">d2l-button-icon</a></li> | |||
| <li><a href="components/button/demo/button-subtle.html">d2l-button-subtle</a></li> | |||
| <li><a href="components/button/demo/button-toggle.html">d2l-button-toggle</a></li> | |||
There was a problem hiding this comment.
There's also the root README where we list out all the things (although I think we should ditch that at some point).
| if (changedProperties.get('pressed') === undefined) return; | ||
|
|
||
| /** Dispatched when the pressed state changes */ | ||
| this.dispatchEvent(new CustomEvent('d2l-button-toggle-change')); |
There was a problem hiding this comment.
For change events, we have just used 'change' as the event name for our form inputs since I think we consider it a more built-in type of event. Could use that here -- I'm good either way.
There was a problem hiding this comment.
Yeah that could certainly work in this case as well. I'll mull it over while I wait for Jeff's input on the docs. Or if anyone else has a preference I am happy to oblige. I know we have a mix across our components, and in some of those cases there may be reasons to be specific.
There was a problem hiding this comment.
I did a little poking around and it seems like the change event was originally intended for form elements INPUT, SELECT, and TEXTAREA. Unless there is any strong opinion, I'll leave this as d2l-button-toggle-change (or just d2l-button-toggle).
|
🎉 This PR is included in version 3.50.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
GAUD-6936
This PR adds a new
d2l-button-togglecomponent. It provides two slots (pressedandnot-pressed) allowing the consumer to place eitherd2l-button-iconord2l-button-subtlewithin.The slot approach was taken (rather than baking the buttons into this component) in order to allow consumers the ability to use different types of buttons (which themselves have different APIs).
Still need docs.