Skip to content

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!

}

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!

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