TextInput: Update TextInput.Action internal component to use Tooltip v2 #4097
Conversation
🦋 Changeset detectedLatest commit: c24979c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
broccolinisoup
changed the title
size-limit report 📦
|
broccolinisoup
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| @@ -93,7 +93,7 @@ const TextInputAction = forwardRef<HTMLButtonElement, TextInputActionProps>( | |||
| return ( | |||
| <Box as="span" className="TextInput-action" marginLeft={1} marginRight={1} lineHeight="0"> | |||
| {icon && !children ? ( | |||
| <Tooltip direction={tooltipDirection} aria-label={ariaLabel}> | |||
| <Tooltip direction={tooltipDirection} text={ariaLabel as unknown as string} type="label"> | |||
There was a problem hiding this comment.
text={ariaLabel as unknown as string} this is not great but some sort of type conversion or "hack" is needed here because text is a required prop for Tooltip but I see we can't make ariaLabel required for TextInput's trailing action because it could be an icon button or a text button. I am open to feedback if we want to handle this in another way
There was a problem hiding this comment.
Would this be a little easier to read/understand?
text={ariaLabel || ''}There was a problem hiding this comment.
Yeah that reads better. Thank you. Although both options will end up rendering an empty tooltip. I wonder if we should conditionally render the tooltip for icon button too? Though this solution will only work until we bring the tooltip to icon buttons by default #2008. What are your thoughts on that?
There was a problem hiding this comment.
Ah ok. Yea, I think we can keep what you have then.
broccolinisoup
temporarily deployed
to
github-pages
GitHub Actions
Inactive
broccolinisoup
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| @@ -93,7 +93,7 @@ const TextInputAction = forwardRef<HTMLButtonElement, TextInputActionProps>( | |||
| return ( | |||
| <Box as="span" className="TextInput-action" marginLeft={1} marginRight={1} lineHeight="0"> | |||
| {icon && !children ? ( | |||
| <Tooltip direction={tooltipDirection} aria-label={ariaLabel}> | |||
| <Tooltip direction={tooltipDirection ?? 's'} text={ariaLabel ?? ''} type="label"> | |||
There was a problem hiding this comment.
Would this be something that we'd want to restrict in v37 so that aria-label is a required prop? I think this is fine since it mirrors the current behavior but in a future major release it'd be great to have this required so we don't have a non-descriptive tooltip
There was a problem hiding this comment.
Would this be something that we'd want to restrict in v37 so that aria-label is a required prop?
I think we could. The only thing I can think of a bit of a blocker is that the TextInput.Action also accepts children and if children is passed, the tooltip becomes optional. (Based on just reading the code). I guess the case would be a text button as trailing action that doesn't need a tooltip. (related additional comment)
Just looking at the current use cases at dotcom, all seem to have icons. I wonder if it would make sense to re-visit the text button use case? 🤔 Or do we have clear uses cases for this? @mperrotti
There was a problem hiding this comment.
I don't think trailing actions need to accept text, just the trailing visuals. For example: we may use trailing visual text to convey units like "minutes". See the timeout settings in https://github.com/settings/codespaces for an example.
There was a problem hiding this comment.
This is great. We can then refactor this conditional tooltip and make aria-label mandatory in the next major. I'll add this to the v37 issue.
broccolinisoup
temporarily deployed
to
github-pages
GitHub Actions
Inactive
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
…v2 (#4097) * Use Tooltip 2 for TextInput.Action * add changeset * snaps and update default direction * update tooltip direction and type casting * accidently removed - bring it back --------- Co-authored-by: Mike Perrotti <mperrotti@github.com>
Closes #4091
Changelog
New
Changed
tooltipDirectionprop tosbecause it is what is defaulted in TooltipRemoved
Rollout strategy
Testing & Reviewing
Changes can be tested in the storybook
Merge checklist