-
Notifications
You must be signed in to change notification settings - Fork 535
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
TextInput: Refactor TextInputInnerAction
to use the default icon button tooltip
#4733
Conversation
🦋 Changeset detectedLatest commit: 69244dd 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 |
size-limit report 📦
|
{icon && !children && ariaLabel ? ( | ||
<IconButton | ||
{...accessibleLabel} | ||
tooltipDirection={tooltipDirection ?? 's'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sparkle @joshblack for helping me fix this obscure type issue ✨
TextInputInnerAction
to use the default icon button tooltip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 clarifying question, LGTM other than that
Approving in advance!
@@ -92,13 +101,28 @@ const TextInputAction = forwardRef<HTMLButtonElement, TextInputActionProps>( | |||
console.warn('Use the `aria-label` prop to provide an accessible label for assistive technology') | |||
} | |||
|
|||
const accessibleLabel = ariaLabel | |||
? {'aria-label': ariaLabel} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives priority to aria-label over aria-labelledby which is opposite of what browsers do. Is that intentional?
aria-labelledby takes precedence over all other methods of providing an accessible name, including aria-label
Ref: https://arc.net/l/quote/ksytdmqp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, kind of.
Because aria-label
is a prop on the TextInputInnerAction
component and it is used to provide tooltip text as well.
If there is aria-labelledby
, the tooltip wiring needs to be manually. So to my understanding, aria-label
is encouraged and preferred.
Let me know if you have any concerns or anything I am missing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see!
Closes #4696
Changelog
Changed
IconButton
in theTextInputInnerAction
component to have the right a11y props (aria-label
andaria-labelledby
) and addtooltipDirection
to specify the direction of the tooltipRemoved
IconButton
in theTextInputInnerAction
Rollout strategy
Testing & Reviewing
Merge checklist