RadioControl: add support for option help text
#63751
Conversation
|
Size Change: +267 B (+0.02%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Enhancement
| // TODO: will improve if we like this solution | ||
| // eslint-disable-next-line no-nested-ternary | ||
| !! option.helpText | ||
| ? `${ id }-${ index }-help` | ||
| : !! help | ||
| ? `${ id }__help` | ||
| : undefined |
There was a problem hiding this comment.
Currently, the option's help text is rendered separately from the label, and is used to describe the associated input element .
Since the help text is not part of the label, clicking it won't select the associated radio.
We could improve this by moving the help text inside the label, but that would make the help text content as part of the label, instead of a description text. We could set aria-hidden="true" on the help text inside the label if we don't want it to "pollute" the input label text, and we could also optionally add a copy of the same text as visually hidden and still use it for the description text of the input.
But before trying this out, I thought I'd start simple and wait for feedback
There was a problem hiding this comment.
Since the help text is not part of the
label, clicking it won't select the associated radio.
I'd say this is the desired behavior. Clicking on help text doesn't (and I'd say shouldn't) focus the control (same in InputControl, CheckboxControl, etc).
There was a problem hiding this comment.
This could be a good opportunity to fix #52890, by changing this component to a proper fieldset markup and associating the group help to that.
If that's out of scope for this PR though, it might be better to keep both the group-level and option-level helps associated if they both exist (apparently you can specify multiple space-delimited ids).
There was a problem hiding this comment.
This could be a good opportunity to fix #52890, by changing this component to a proper fieldset markup and associating the group help to that.
That would be great, but do you know if BaseControl supports using fieldset ?
If that's out of scope for this PR though, it might be better to keep both the group-level and option-level helps associated if they both exist (apparently you can specify multiple space-delimited ids).
That's what I've done for now.
There was a problem hiding this comment.
That would be great, but do you know if
BaseControlsupports usingfieldset?
No, we currently need to do custom implementations using BaseControl.VisualLabel. But baking fieldset support into BaseControl might be something to consider! (Not immediately sure if that's a good idea)
There was a problem hiding this comment.
No, we currently need to do custom implementations using
BaseControl.VisualLabel.
Ok, this definitely sounds to me like a task that should have its dedicated PR. I will not work on this in this PR.
But baking
fieldsetsupport into BaseControl might be something to consider! (Not immediately sure if that's a good idea)
I think we could give it a try, in a new version of BaseControl. I had open a related issue some time ago, and I think Andrew at some point even came up with a proof of concept.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jarekmorawski. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| // TODO: will improve if we like this solution | ||
| // eslint-disable-next-line no-nested-ternary | ||
| !! option.helpText | ||
| ? `${ id }-${ index }-help` | ||
| : !! help | ||
| ? `${ id }__help` | ||
| : undefined |
There was a problem hiding this comment.
Since the help text is not part of the
label, clicking it won't select the associated radio.
I'd say this is the desired behavior. Clicking on help text doesn't (and I'd say shouldn't) focus the control (same in InputControl, CheckboxControl, etc).
| // TODO: will improve if we like this solution | ||
| // eslint-disable-next-line no-nested-ternary | ||
| !! option.helpText | ||
| ? `${ id }-${ index }-help` | ||
| : !! help | ||
| ? `${ id }__help` | ||
| : undefined |
There was a problem hiding this comment.
This could be a good opportunity to fix #52890, by changing this component to a proper fieldset markup and associating the group help to that.
If that's out of scope for this PR though, it might be better to keep both the group-level and option-level helps associated if they both exist (apparently you can specify multiple space-delimited ids).
ciampo
force-pushed
the
feat/radio-control-option-help-text
branch
from
27eafab
to
7c60320
Compare
|
Update: I addressed most of the feedback. Pending items:
Follow-up:
|
I reckon we should align with the recent updates to CheckboxControl and ToggleControl, so let's try 8px between label + help text. We can also increase the gap between radios to 12px. One other observation; when no help text is supplied, the RadioControl height is increased by 4px, probably due to the 
Ideally |
|
@jameskoster thank you for the feedback — I changed the horizontal gap between radio and label to
Personally, it feels like the vertical space if a bit off — the gap between the label and help text seems too large, compared to the gap between each radio option and between the radio group and the group help text. But this is just my opinion, and I'd be fine with keeping things as they are. |
ciampo
force-pushed
the
feat/radio-control-option-help-text
branch
from
1e05029
to
cd5cb27
Compare
|
Hmm yes that doesn't look quite right. The gap between label + help text should be consistent across radios, checkboxes, and toggles. In this context I agree it looks a little large, perhaps we reduce that to In Storybook I'm only seeing an Increasing to 
|
ciampo
force-pushed
the
feat/radio-control-option-help-text
branch
2 times, most recently
from
81f5396
to
5ecc65e
Compare
|
Agreed. Pushed a commit with the tweaked spacing
|
|
This PR is ready for another round of review. |
|
Curious. Does help text at the bottom of a set of radio buttons make sense? Seems more connective to the label than to the end of the elements. |
💯 That'll be changed in #63734. |
|
Looking good to me. I think the gap between the heading and the group could be increased, but it's probably fine to do that in #63734. Would it make sense to update the gap between label + help text for checkboxes and toggles here too? I wonder if we should add a variable for this... |
ciampo
force-pushed
the
feat/radio-control-option-help-text
branch
from
bb3aa5c
to
391bc27
Compare
|
@mirka all existing feedback should have beeen addressed.
I'd prefer to merge this PR soon and focus on tweaking spacing and/or aligning it with other components in separate PRs. |
What?
Closes #63735
Add support for help text to individual radio options in the
RadioControlcomponentWhy?
How?
options[].descriptionpropertyaria-describedbyattribute)Follow-ups
fieldsetto properly label the radio group (RadioControl: add support for option help text #63751 (comment));BaseControlso that itsStyledHelpsub-component doesn't come with a top margin.Testing Instructions
options[].descriptionproperty. Make sure that the radio inputs continue working as expectedScreenshots or screencast