-
Notifications
You must be signed in to change notification settings - Fork 536
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
Tooltip: Do not wrap the tooltip and the trigger in a div #4056
Conversation
🦋 Changeset detectedLatest commit: 2e500db 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 📦
|
@@ -34,6 +34,7 @@ const InactiveIndicator: React.FC<{ | |||
padding: 0, | |||
font: 'inherit', | |||
cursor: 'pointer', | |||
display: 'flex', |
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.
Due to removing the wrapper in the tooltip, I needed to add this to align the inactive visual in center. Let me know if you prefer a different way to do it @mperrotti
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.
Definitely makes sense to avoid an extra wrapper!
Am I right in assuming that because tooltip is now in top layer, removing this wrapper makes no difference to positioning?
Yes! Because we use popover, tooltip is always on the top layer. Also because we use anchor positioning to set top and left for the tooltip div, it makes no difference how it is placed in the dom. The tests and stories are looking too ✨ @siddharthkp |
Changelog
New
Changed
Removed the div wrapper because it was causing styling issues on the icon buttons (WIP PR) and added mouse enter and leave events to the tooltip to make sure it stays open when it is hovered. I don't think we need it but let me know if I am missing anything.
Removed
Rollout strategy
Testing & Reviewing
Merge checklist