-
Notifications
You must be signed in to change notification settings - Fork 842
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
Tooltips Style #193
Tooltips Style #193
Conversation
- Re-worked Tooltip.js to conform to component style guide – Styling is all done except for positioning and transitions – TooltipTrigger.js still needs to be re-worked
@snide Please see if you can figure out how to position this thing against the trigger element via CSS only... |
- Animations transform the tooltip but don’t position it - Arrow now pointing in the correct direction
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.
This code is sooooo much cleaner. Nice work. Had some minor styling comments. Feel free to adjust them how you think it would work best and merge away.
src/components/tooltip/_tooltip.scss
Outdated
|
||
&.tooltip-container-visible { | ||
// ANIMATING | ||
transition: |
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'd add a transition delay here. Just something so that the tooltip doesn't casually show if you're waving your mouse around a page. It's an easy way to make them not annoying and more directed.
src/components/tooltip/_tooltip.scss
Outdated
&.tooltip-container-visible { | ||
// ANIMATING | ||
transition: | ||
opacity $euiAnimSlightBounce $euiAnimSpeedSlow, |
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'd up the speed to normal maybe? Then pair it with a shorter travel time. Feels less like it's flying over. Should be subtle.
src/components/tooltip/_tooltip.scss
Outdated
transform $euiAnimSlightBounce $euiAnimSpeedSlow; | ||
opacity: 0; | ||
visibility: hidden; | ||
transform: translateX(0) translateY($euiSizeL * -1) translateY(0); |
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.
Think along with the quicker animation speed, if you cut the travel to something like $euiSizeS
it might look a little tighter.
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.
Makes sense, I'll adjust all the timings here. I mainly just copied from the popover, but I suppose you're right about it being a different interaction from that and so needing a different animation.
.euiTooltip__content { | ||
|
||
// Scoped variables for component-only re-use | ||
$background-color: $euiColorDarkestShade; |
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.
Generally I've avoided component variable tokenization, mostly because our theming system is so ridgid we kind of need people to use the globals. For example, if they changed this to a hex value, theming would break it dark mode.
If you think it's important prefix it with the component name $euiTooltipBackgroundColor
...etc so you don't fun into conflicts since we generate a big sass file right now.
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.
Since these variables are within the .euiTooltip__content{}
they're not known to anything outside of this selector (and this particular section of the selector) so we won't run into any namespacing issues.
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.
Nice. 👍
src/components/tooltip/_tooltip.scss
Outdated
// The tooltip title if it exists | ||
.euiTooltip__title { | ||
@include euiFontSizeM; | ||
font-weight: $euiFontWeightLight; |
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.
Blargh, this title is tough to read and it's totally my fault and I know you're copying what I had. Come up with something cooler! Should our small titles use bold? Probably? I dunno. Maybe adjust the color if we use the light / same size as the body? Maybe the body text just needs to be smaller and it would look fine?
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.
Yeah I felt the same way. I can re-style.
- Animations just ease-out with a slight delay (unless it’s a click action) - Title looks more like a title - Adjusted the overall <strong> elements and added bold weight of 700
Initial re-work done