Tooltip v2: Allow external id to be passed down in tooltip so that the trigger can be labelled/described by the that id #4200
Conversation
🦋 Changeset detectedLatest commit: c93cffe 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
requested review from
a team,
joshblack,
pksjce and
siddharthkp
joshblack
left a comment
joshblack
left a comment
There was a problem hiding this comment.
Looks great! Just had a question that I left as a comment and also wanted to ask if there was a test case that would cover this, as well 👀
src/drafts/Tooltip/Tooltip.tsx
Outdated
| export const Tooltip = React.forwardRef( | ||
| ({direction = 's', text, type = 'description', children, ...rest}: TooltipProps, forwardedRef) => { | ||
| const tooltipId = useId() | ||
| const tooltipId = useId(rest.id) |
There was a problem hiding this comment.
Wanted to ask, is there a specific reason to not destructure this off of props? It doesn't seem like id as an identifier is used and could prevent potential bugs in the future if the position of {...rest} changes (like if someone accidentally moves it after where id={tooltipId} is set.
There was a problem hiding this comment.
Great point! I used rest.id because id isn't a defined prop like text or children but I guess it doesn't matter though and it is not very safe. Thanks for the comment! I updated it and added tests ✨ c93cffe
broccolinisoup
force-pushed
the
tooltip-v2-allow-external-id
branch
from
ee85bc2 to
c93cffe
Compare
Changelog
New
Allow Tooltip to use the custom id to label/describe the trigger element
Changed
Removed
Rollout strategy
Testing & Reviewing
In the story, you can confirm that the custom id that is passed in the code is present in the dom.
Merge checklist