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

TextInput: Refactor TextInputInnerAction to use the default icon button tooltip #4733

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

broccolinisoup
Copy link
Member

@broccolinisoup broccolinisoup commented Jul 11, 2024

Closes #4696

Changelog

Changed

  • Updated IconButton in the TextInputInnerAction component to have the right a11y props (aria-label and aria-labelledby) and add tooltipDirection to specify the direction of the tooltip

Removed

  • External Tooltip that wraps the IconButton in the TextInputInnerAction

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Jul 11, 2024

🦋 Changeset detected

Latest commit: 69244dd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@github-actions github-actions bot added the staff Author is a staff member label Jul 11, 2024
Copy link
Contributor

github-actions bot commented Jul 11, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 91.22 KB (+0.09% 🔺)
packages/react/dist/browser.umd.js 91.47 KB (-0.02% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-4733 July 11, 2024 02:26 Inactive
{icon && !children && ariaLabel ? (
<IconButton
{...accessibleLabel}
tooltipDirection={tooltipDirection ?? 's'}
Copy link
Member Author

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 ✨

@broccolinisoup broccolinisoup changed the title Remove the external Tooltip from textinputinneraction TextInput: Refactor TextInputInnerAction to use the default icon button tooltip Jul 11, 2024
Copy link
Member

@siddharthkp siddharthkp left a 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}
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see!

@broccolinisoup broccolinisoup added this pull request to the merge queue Jul 23, 2024
Merged via the queue into main with commit 1c131f8 Jul 23, 2024
30 checks passed
@broccolinisoup broccolinisoup deleted the use-default-tooltip branch July 23, 2024 01:07
@primer primer bot mentioned this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
staff Author is a staff member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Roll out tooltip in our primer/react components that uses IconButton under the hood
2 participants