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

Checkbox/radio - support for isLabelWrapped #9710

Closed
mcoker opened this issue Oct 4, 2023 · 1 comment · Fixed by #9830
Closed

Checkbox/radio - support for isLabelWrapped #9710

mcoker opened this issue Oct 4, 2023 · 1 comment · Fixed by #9830
Assignees

Comments

@mcoker
Copy link
Contributor

mcoker commented Oct 4, 2023

The radio component supports isLabelWrapped, which uses a <label> as the outer/parent component wrapper for the radio component, and changes the inner "label" element to a <span> so that the markup is <label><input><span>label</span></label> instead of <div><input><label>label</label></div>, and the checkbox does not.

The checkbox supports a component prop that will change the outer/parent component wrapper to whatever (eg, component="label" renders it as a <label>), but the inner "label" element is fixed and is always a <label>, so if you do <Checkbox component="label"> then you'll have a nested <label> inside, too, which you don't want. The radio component does not support component. You can see the issue with the nested <label> here:

Screenshot 2023-10-04 at 11 37 49 AM

Also we have a use case for a checkbox with the parent wrapper as a <label> in the <MenuToggleCheckbox /> component, but you can see we've manually recreated the checkbox component to get the correct HTML structure (there may be other reasons it was made this way). Ideally, <MenuToggleCheckbox /> could just be <Checkbox isLabelWrapped />.

My recommendation:

  • Add support for <Checkbox isLabelWrapped />
  • Update <MenuToggleCheckbox /> to use ^^
  • Be consistent in the support for the component prop for radio/checkbox, and if we support it, be consistent on both the parent wrapper and the "label" wrapper.
    • I think being able to specify a different element as the outer/parent wrapper is good using component
    • Since the label prop for both components is a ReactNode, I don't know how necessary it is to allow users to change the "label" element wrapper, since they could just pass whatever kind of element they want to label. But if it doesn't hurt anything, it may be good to support some kind of labelComponent prop or something for it anyways.
Copy link

stale bot commented Jan 1, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix label Jan 1, 2024
@adamviktora adamviktora moved this from Backlog to PR Review in PatternFly Issues Jan 2, 2024
@github-project-automation github-project-automation bot moved this from PR Review to Done in PatternFly Issues Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants