Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

docs(Checkbox): add examples of reversed and label wraps#10318

Merged
tlabaj merged 2 commits intopatternfly:mainfrom
adamviktora:checkbox-examples
May 10, 2024
Merged

docs(Checkbox): add examples of reversed and label wraps#10318
tlabaj merged 2 commits intopatternfly:mainfrom
adamviktora:checkbox-examples

Conversation

Copy link
Contributor

adamviktora commented Apr 30, 2024

What: Closes #10085

adamviktora requested review from a team, kmcfaul and tlabaj and removed request for a team April 30, 2024 13:36
Copy link
Collaborator

patternfly-build commented Apr 30, 2024 *
edited
Loading

thatblindgeye requested changes May 7, 2024

### 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.
Copy link
Contributor

thatblindgeye May 7, 2024

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.

Suggested change
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?

Copy link
Contributor

edonehoo May 7, 2024

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 component"

(of course, all of this ^ may need to be combined with what you said/remixed for accuracy)

Copy link
Contributor

thatblindgeye May 8, 2024

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 would render:

<label for="test">Somethinglabel>
<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"

edonehoo reacted with heart emoji
Copy link
Contributor

edonehoo May 8, 2024

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

Copy link
Contributor Author

adamviktora May 9, 2024

Choose a reason for hiding this comment

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

@thatblindgeye

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)

Copy link
Contributor

thatblindgeye May 9, 2024

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.

Copy link
Contributor

edonehoo May 9, 2024 *
edited
Loading

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.

adamviktora reacted with heart emoji
kmcfaul approved these changes May 8, 2024
Copy link
Contributor

kmcfaul left a 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).

adamviktora force-pushed the checkbox-examples branch from a9f4f43 to f5dc5de Compare May 10, 2024 09:17
adamviktora force-pushed the checkbox-examples branch from f5dc5de to f3083ea Compare May 10, 2024 09:18
adamviktora requested review from edonehoo and thatblindgeye May 10, 2024 09:19
thatblindgeye approved these changes May 10, 2024
edonehoo approved these changes May 10, 2024
tlabaj approved these changes May 10, 2024
tlabaj merged commit 6d5bde2 into patternfly:main May 10, 2024
Copy link
Collaborator

patternfly-build commented May 10, 2024

Your changes have been released in:

  • @patternfly/react-code-editor@5.4.0-prerelease.10
  • @patternfly/react-core@5.4.0-prerelease.10
  • @patternfly/react-docs@6.4.0-prerelease.13
  • @patternfly/react-drag-drop@5.4.0-prerelease.11
  • demo-app-ts@5.1.1-prerelease.111
  • @patternfly/react-table@5.4.0-prerelease.10
  • @patternfly/react-templates@1.1.0-prerelease.10

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tlabaj tlabaj approved these changes

kmcfaul kmcfaul approved these changes

thatblindgeye thatblindgeye approved these changes

+1 more reviewer

edonehoo edonehoo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Checkbox - add examples of "Reversed" and "Label wraps"

6 participants