fix(MenuToggle): removed nested label elements#6923
Conversation
|
Preview: https://patternfly-pr-6923.surge.sh A11y report: https://patternfly-pr-6923-a11y.surge.sh |
| ### Split button (checkbox with label) | ||
| ### Split toggle with labeled checkbox | ||
|
|
||
| To add a label to a split toggle that will be linked to the checkbox, pass the text wrapped in `span.pf-v6-c-check__label` as a child to `label.pf-v6-c-check`. Clicking the text will update the checked state of the split toggle's checkbox. |
There was a problem hiding this comment.
Not sure since I can't load the surge preview right now to interact with this example, but the React version of this example doesn't update the checkbox when you click the text -- is this one different?
There was a problem hiding this comment.
Yeah, for this PR this example updates the checkbox when you click the text, while "Split toggle with checkbox and toggle text label" in React is the one that updates the checkbox when you click the text. So this PR is basically swapping the behaviors between the 2 examples ("Split toggle with checkbox and toggle text"/"Split toggle with checkbox and toggle text label" and "Split toggle with labeled checkbox")
If it makes more sense how React is setup I can swap them here, though.
There was a problem hiding this comment.
Ah okay, gotcha! I think? The naming of these in general is tough 😅
I think that we should try to match with v6 React, whether that means changing the React example names in a followup, or aligning in this pr. I personally think the language of "labeled checkbox" doesn't signify that clicking the text triggers selection as well as saying "toggle text" does. Or maybe even "label that toggles", "clickable text/label", "selectable text/label"?
But, I think I'm still confused. Your "Split toggle with checkbox and toggle text" example also says "Clicking the text should trigger any click action on the toggle." Is that new behavior, since the respective React "Split toggle with labeled checkbox" example doesn't trigger any action on clicks? Am I still mixing them up lol
There was a problem hiding this comment.
The naming of these in general is tough
You are not wrong 😆
Basically with this PR I was trying to convey that "Split toggle with labelled checkbox" ties the text label to the checkbox, and clicking the text will trigger the checkbox to be updated. Then "Split toggle with checkbox and toggle text" ties the text label to the toggle button. The "trigger click action on the toggle" is technically the default behavior of the split toggle, I don't think we actually show that in the example(s), though, since they're just stand alone Menu Toggles.
But yeah as long as we have Core and React matching in the end. It just depends what makes the most sense, which unfortunately is tough 😆
There was a problem hiding this comment.
Okay, now I'm finally following -- ty for explaining so much 😁 In that case, I've changed my mind. I think the example titles that you use in this PR make the most sense! I can open up a followup pr to update the React naming to match
|
🎉 This PR is included in version 6.0.0-alpha.208 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #6776
Also updated some labeling in examples. We should also update other examples/demos (post v6) so that any dynamic text inside a split toggle isn't tied to the checkbox input itself. Otherwise it runs into a similar issue that we had to fix for the Switch, where the dynamic labeling will cause confusion.
For example, the Bulk Select React example updates the input's labeling to "Deselect all cards" when at least 1 item is selected; if all items are selected, the labeling conveys that all cards are already deselected ("deselect all cards, checked checkbox"), when the opposite is actually true (all cards are selected, but clicking the checkbox will cause all cards to be deselected).
Likewise, if the text is tied to the checkbox and it's constantly being updated ("1 selected", "5 selected", etc), assuming a total of 10 items, it being announced "1 selected, mixed checkbox" and "5 selected, mixed checkbox" may not be totally clear. Are 5 items currently selected, or are some of the 5 total items selected (which may be more confusing if there's actually more than 5 items total)? Maybe having the text be the input's description would be better (it'd then announce something like "Select all items, 5 selected, mixed checkbox").
cc @kmcfaul @edonehoo regarding the example descriptions I added based on the React split toggle examples. Wasn't sure if I got the descriptions + naming right or if the React examples are reversed?