InputControl: Add help prop
#45931
Merged
InputControl: Add help prop
#45931
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Size Change: +20.5 kB (+2%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
|
Instead of making |
My bad, I was overestimating the effort required to make this happen, at least in the context of this PR. It is still a bit high-effort to consolidate the I changed this PR to use BaseControl 👍 |
ciampo
approved these changes
Nov 26, 2022
There was a problem hiding this comment.
Nicely done! Tested in Storybook (both InputControl / NumberControl / UnitControl), works as expected 🚀
It would be even better if InputControl could leverage BaseControl fully, including label and hideLabelFromVision.
Maybe a good follow-up would be to bring the labelPosition feature to BaseControl, and then refactor InputControl removing its custom implementation of label and using instead BaseControl ?
| /** | ||
| * An additional description for the control. | ||
| */ | ||
| help?: string; |
There was a problem hiding this comment.
BaseControl's help prop is a ReactNode — is there any specific reason why we're using string instead of Pick< BaseControlProps, 'help' > ?
There was a problem hiding this comment.
This was because the help implementation in BaseControl was not marked up semantically, as @tellthemachines noted. ARIA descriptions actually require plain text. I was kind of considering removing ReactNode support in BaseControl itself in the process of fixing the markup issue.
However, taking a better look at the codebase, there are already a good number of help usage that include links in the text. So I'm going to suggest that we mark it up as aria-describedby when plain text, and fall back to aria-details when not. e4806bb
I believe it's still preferable to use aria-describedby because aria-details support is pretty bad.
There was a problem hiding this comment.
Agreed, thank you for the context! Should we mention that a plain string of text is preferable in the help prop description in BaseControl ?
There was a problem hiding this comment.
Ok, I'll add that in the follow-up when I add the semantics to BaseControl 👍
tellthemachines
approved these changes
Nov 28, 2022
There was a problem hiding this comment.
Thanks for fixing this! I tested it with VoiceOver and the description is being read out correctly when the input is focused, thanks to the use of aria-describedby ✅
While working on #45364, I noticed that when adding help to the ToggleGroupControl, there is no semantic link between the text and the control. Could we fix that (as a separate task, of course!) so that ToggleGroupControl also uses aria-describedby for its help text?
That would be nice, but realistically my current assessment is that it's hard to justify the effort, given our current priorities 🙁 It's not a quick low-risk refactor given the surface area, and the benefits are not very substantial at the moment. Maybe when the |
Yes, this is next on my list 👍 I'll backport the |
This was referenced Nov 30, 2022
This was referenced Apr 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Required for #45364 (comment)
What?
Adds a
helpprop, similar to the one for BaseControl, on the InputControl component and friends (NumberControl, UnitControl).Why?
InputControl does not use the standard BaseControl, in order to support the
labelPositionfeature. Therefore, it needs its ownhelpimplementation.How?
Reuse the
StyledHelpfrom BaseControl and add semantics.Testing Instructions
npm run storybook:devlabelPositionprop's value.Screenshots or screencast