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 updates #2135
Tooltip updates #2135
Conversation
Created a component that supportss two types of tooltip based on the Figma views. The first is a standart tooltip that looks like a speech bubble. The second type is more complex case, where contains header and content.
We need to `clearTimeout` in the `useEffect` hook to prevent memory leaks.
Here we added `onMouseEnter` and `onMouseLeave` event to the tooltip content wrapper to stick the tooltip content when a user hovers on the content.
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.
Left some notes, but nothing blocking. Gave it a whirl, was thrown by the balance display being at 1000 KEEP then realized there's another PR actually hooking that number up to reality 😁
className={`address-shortcut tooltip address ${classNames}`} | ||
<Tooltip | ||
simple | ||
triggerComponent={() => { |
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.
Hmm… This feels like a bit of an inversion of control. How hard would it be to have something that looks more like:
const copyTooltip = <Tooltip simple>{copyStatus}</Tooltip>
<button
onMouseOver={showTooltip(copyTooltip)}
onMouseOut={hideTooltip(copyTooltip)}
onClick={copyToClipboard}
className={...}>{addr}</button>
? In this scenario showTooltip
and hideTooltip
might be imported from the tooltip's component file as well.
That seems to more obviously define the relationship between the button and the tooltip in my mind---but I realize this is all a little more complicated than it may look at first blush.
Since there's some useState
in the mix here, perhaps it's a useSimpleTooltip(copyStatus)
instead of a <Tooltip...>
that also sets the show and hide functions, or a TooltipButton
component that wraps the state management… e.g.:
const [showCopy, hideCopy] = useSimpleTooltip(copyStatus)
<button
onMouseOver={showCopy}
onMouseOut={hideCopy}
onClick={copyToClipboard}
className={...}>{addr}</button>
Or for this particular case:
<TooltipButton simple onClick={copyToClipboard} className={...}>{addr}</TooltipButton>
In general I personally find that any pattern that passes a parent or sibling to its child for rendering is a bit confusing to follow, and there's often a way to step out of the problem and figure out a way to make it adhere to the hierarchical expectations of components a little bit better.
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.
Hmm to be honest not follow. A Tooltip component handle all "logic" about showing and hiding tooltip. So we just pass a trigger component instead of handling feature outside tooltip component.
In that case you described we would have to pass a component to render to showCopy
function and take care of hiding/showing every time when we want to use tooltip component.
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.
My conceptualization here is that where the tooltip is triggered is not a concern of the tooltip, it's a concern of the caller using the tooltip—something I feel is hinted at by the hierarchy, where the caller is above the tooltip, and the trigger is never below the tooltip. In this formulation, the tooltip component provides all the tools to present tooltips, including functions to show and hide it, but actually hooking that up is something left to the caller.
import * as Icons from "./Icons" | ||
|
||
const Tooltip = ({ | ||
triggerComponent: TriggerComponent, |
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.
🤔 wait does our build support TypeScript already? Have I already asked this? Hahaha.
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.
heh
It's about assigning a new name to variable.
From docs:
A property can be unpacked from an object and assigned to a variable with a different name than the object property.
const o = {p: 42, q: true}; const {p: foo, q: bar} = o; console.log(foo); // 42 console.log(bar); // true
Here, for example, const {p: foo} = o takes from the object o the property named p and assigns it to a local variable named foo.
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.
Aaaand I missed the curly braces on the opening line 😂
) : ( | ||
<> | ||
<Tooltip.Header text={title} icon={IconComponent} /> | ||
<Tooltip.Divider /> |
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 overall structure here seems a little more complicated that it might need to be for what we're trying to achieve… Nothing urgent, but would be nice to revisit at some point from the perspective of how we can use semantic markup and targeted styling to potentially collapse some of the layers a little bit.
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 main idea was to achieve maximum flexibility in case to create custom tooltip components via passing children
and all necessary "logic" is inside tooltip component. But if skip children
and pass title
, IconComponent
and content
props we render the default tooltip component from KEEP Design System
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.
Do we use that flexibility right now, or is it more hypothetical?
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.
Probably more hypothetical, but we have some tooltips with redirect button and some without. Also, I think is better to have this flexibility instead of passing props like: withBtn
, topBtnProps
, bottomBtnProps
etc. in case we would add a new type of tooltip.
Maybe we should just leave {children}
and create something like ResourceTooltip
component that would render this:
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.
That could be an interesting alternative to look into, definitely!
Closes: #2079
This PR updates the
Tooltip
component based on the Figma views.