-
Notifications
You must be signed in to change notification settings - Fork 355
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
docs(Checkbox): add examples of reversed and label wraps #10318
Conversation
Preview: https://patternfly-react-pr-10318.surge.sh A11y report: https://patternfly-react-pr-10318-a11y.surge.sh |
|
||
### Label wraps | ||
|
||
When the input is wrapped in a label, larger area can be clicked to check the box, including the space between the checkbox and its description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be true even for a non-wrapped input, if the id
prop is passed in. The/one of the main benefits for wrapping the input in a label is that we don't need to pass an id
into the input nor the for
attribute to the label.
When the input is wrapped in a label, larger area can be clicked to check the box, including the space between the checkbox and its description. | |
When the input is wrapped by a `label` element, an `id` does not need to be passed into the input and the `label` element does not need a `for` prop passed in. |
cc @edonehoo wdyt of the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our recent content research study suggested that people liked having a non-technical description ahead of tech specifics, so I would also consider adding something like "You can increase the size of a checkbox's clickable area to span wider than the checkbox label." and a line break prior to this sentence.
My usual string of multiple questions: Should we also mention the isLabelWrapped
prop? Does this prop handle what you described behind the scenes? Where does the for
attribute come into play (since I'm not seeing it in the other examples to contrast)?
If so, it may be more helpful to say something like:
"To expand the clickable area, add the isLabelWrapped
prop. This wraps the checkbox input with a label
element, which removes the need to pass id
to the <Checkbox>
component"
(of course, all of this ^ may need to be combined with what you said/remixed for accuracy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edonehoo the checkbox and radio components are a little confusing at first I think.
The for
property doesn't need to be passed in, but rather passing in an id
will set the for
property internally. So something like <Checkbox id="test" labelText="Something" />
would render:
<label for="test">Something</label>
<input type="checkbox" id="test" />
(or something like that)
I think your last suggestion looks good, maybe just rearranging a little: "To expand the clickable area without needing to pass an id
in, add the isLabelWrapped prop."
Not a blocker, though, more nitpicky 😆 I may be overly worried about conveying the idea that "the only way to expand the clickable area is with isLabelWrapped"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see! Thanks for explaining! In that case, I think your rearranged suggestion sounds great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be true even for a non-wrapped input, if the id prop is passed in.
Is the behavior when passing id
vs isLabelWrapped
really the same? What I meant by the description is that the space between the text and checkbox is also clickable.
Here is an example with using id:
Screen.Recording.2024-05-09.at.17.26.53.mov
And here with isLabelWrapped:
Screen.Recording.2024-05-09.at.17.27.43.mov
Good example is also this: https://ux.stackexchange.com/a/47920
I really like both your's and Erin's descriptions, I just thought we should say why is wrapping in a label different than using just the id
(with the internal for
attribute)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I misread then I think, I see what you mean now. Since like you commented below id
is required, maybe we could combine your original comment plus the above, something like "To expand the clickable area, add the isLabelWrapped
property. This will allow a checkbox description or the space between the input and label to be clickable."? @edonehoo how does that sound, and do you think the example should include a description to better convey the behavior?
We should keep id
required, as even when the label
element wraps the input it could still help prevent any issues by explicitly linking the two via for
and id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm if the space between the checkbox and label is specifically important to call out, then I think that sounds good. I stuck with the more generic "expand the clickable area" because it seems like you can also click to the right of the label to select it, so I wanted to cover both cases. WDYT?
I would definitely add an example description, so putting it all together, maybe something like this:
You can expand the clickable area of a checkbox so that it spans wider than the checkbox label. This allows users to select a checkbox by clicking the checkbox itself, the label, or the area between the checkbox and the label.
To expand the clickable area of a checkbox, add the
isLabelWrapped
property.
packages/react-core/src/components/Checkbox/examples/CheckboxLabelWraps.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm just needs the doc wording update Erin mentioned (or at least a followup opened to update the wording).
a9f4f43
to
f5dc5de
Compare
f5dc5de
to
f3083ea
Compare
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #10085