-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Tooltip design fixes #1515
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 design fixes #1515
Conversation
|
Build successful! 🎉 |
| import {useTooltipTriggerState} from '@react-stately/tooltip'; | ||
|
|
||
| const DEFAULT_OFFSET = 7; // Closest visual match to Spectrum's mocks | ||
| const DEFAULT_OFFSET = -1; // Closest visual match to Spectrum's mocks |
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.
why negative?
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.
the design feedback wanted a distance of 4px between the button and the tooltip while we had a distance of 12px, hence the 7 - 8 = -1 for the default offset here. Didn't want to modify useOverlayPosition's math since that would affect other overlays so went for this default offset modification since we already had it defined
| margin-left: var(--spectrum-tooltip-target-offset); | ||
| &.is-open { | ||
| @inherit: %spectrum-overlay--right--open; | ||
| /* @inherit: %spectrum-overlay--right--open; */ |
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.
interesting, what was this? and why only comment it out?
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.
ah whoops, missed that, will fix
| import {useTooltipTriggerState} from '@react-stately/tooltip'; | ||
|
|
||
| const DEFAULT_OFFSET = 7; // Closest visual match to Spectrum's mocks | ||
| const DEFAULT_OFFSET = -1; // Closest visual match to Spectrum's mocks |
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.
should we change the comment now? I'm assuming this is being added to something
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 forgot to submit my review, you answered the question for @dannify but could you make it a comment?
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.
Sure, will do
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
Closes #1445
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: