-
-
Notifications
You must be signed in to change notification settings - Fork 28
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] Component and Hook #264
base: master
Are you sure you want to change the base?
Conversation
@atomiks Hilarious to see this in one day 🤣🔥
|
Yeah 😄 as the review comment above says, it's really 2 years of work, compressed into 1 day since I can just reuse all that work I did. |
|
The main use case where this is necessary for touch input is the "infotip" pattern, where an info icon can be tapped to reveal information. It's not a button tool that performs an action, as the action itself is to show the infotip. For keyboard usage, this pattern may require a Another potential solution is to use long press to show the tooltip on touch devices, rather than a tap. This lets them see the description without triggering the button. Probably somewhat rare? |
8d4d38b
to
1efbbb8
Compare
22e2843
to
11f1654
Compare
4966245
to
b3f0b25
Compare
Thanks for the spot - fixed |
return ( | ||
<Tooltip.Root> | ||
<AnchorButton>Anchor</AnchorButton> | ||
<TooltipPopup sideOffset={7}> |
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 first thought when I started playing with it was that I shouldn't need to manually sync the arrow size with sideOffset
and having the arrow present should automatically move the tooltip away from the anchor so the arrow fits.
I understand it could ber tricky to determine the actual size of the arrow, though, especially if transformed HTML elements are used.
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.
Sometimes they might want a gap between the trigger and the tooltip arrow, not only matching how tall the arrow is. Another thing is the border-radius
isn't avoided intelligently either (uses a static 5px
padding), but Floating UI or Popper never supported this type of auto-detection, only providing a value to manually specify the padding. (We didn't really try to see if it can work well though.)
exiting: boolean; | ||
}; | ||
|
||
export interface TooltipPopupProps |
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'm not sure where it's coming from, but VSCode suggests setMounted
as a Popup prop. It doesn't seem correct (onMounted
would be better if we need to expose it)
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.
It was due to extending TooltipPopupParameters
(the hook's low-level params) without omitting them. I've updated the type
No, mistake when adding the |
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 group's delay still needs some work. It appears that ther delay
prop controls the closing delay (see https://codesandbox.io/p/sandbox/stupefied-kirch-trdy5s?file=%2Fsrc%2FDemo.tsx%3A13%2C23)
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.
In #405 I ended up using a single file for both component and hook types.
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 contexts and their types should be exported as well.
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 is currently pending the decision about contexts and hooks consuming them. Will leave them out for now.
|
||
const contextValue = React.useMemo( | ||
() => ({ | ||
delay, |
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.
It might be worth showing a console warning in development when delay-related props are set in both the Root and Group.
A quick feedback:
|
They can use a global provider if they want (the rest of the app remains performant). A provider allow them to change the props for a certain group of tooltips by overriding a "global" one if necessary. Keeping it fully global goes against React's model for the most part.
We've completely re-thought about what tooltips should be. It does not work on mobile intentionally as tooltips are an anti-pattern on touch devices. The docs explain this further and what to use instead.
These are common issues we've thought of so far and we've either avoided them or know that they require the user to workaround it explicitly in their code |
Tooltip.Group
Delay
Docs Content
|
It's a bit confusing in
The thing with the |
Closes mui/material-ui#32
Preview: https://deploy-preview-264--base-ui.netlify.app/base-ui/react-tooltip/
Transition experiment: https://deploy-preview-264--base-ui.netlify.app/experiments/tooltip/
TODO: