Skip to content
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

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Apr 3, 2024

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:

  • Tests - a lot of the implementation details are tested by Floating UI already, so we only need to test the wrapping APIs.
  • Complete documentation

@atomiks atomiks marked this pull request as draft April 3, 2024 08:13
@atomiks atomiks added the component: tooltip This is the name of the generic UI component, not the React module! label Apr 3, 2024
@colmtuite
Copy link

colmtuite commented Apr 3, 2024

@atomiks Hilarious to see this in one day 🤣🔥

  1. Tooltip should close then you click whatever is inside Anchor.
  2. We should specify a default delay, something around 750ms.
  3. I'm not a fan of the object syntax for setting open and close inside the same delay prop. One reason is that it's not intuitive that setting delay also sets the close delay. Another is just the awkward ergonomics. I'd rather two separate props. However, it's also worth considering not supporting a close delay at all. Can you think of a reason why one is necessary?
  4. When a tooltip is open, and I very quickly hover another element with a tooltip, the next tooltip should open instantly with no delay, regardless of its delay setting. There should be ms threshold for how much time must pass after the first tooltip closes, before you open the second tooltip, for its delay to be removed. Radix calls this skipDelayDuration and it's configurable. RA calls it "cooldown period" and it looks like it's not configurable. I'm guessing since we're not using a Tooltip Provider, and each tooltip is responsible only for its own settings, we can't configure this globally across all tooltips? If that's the case, then I think it's probably ok that it's not configurable.
  5. Do we need a global Tooltip Provider? Related to the above point.
  6. anchorGap is confusing to me, since when I read it, I'm not 100% sure what "anchor" or "gap" means. I think "offset" is a more appropriate name.
  7. I think anchorGap | offset should be on Content rather than Root.
  8. What is the benefit of the allowTouch prop? Is there an issue with just having tooltips not work on touch devices?

@atomiks
Copy link
Contributor Author

atomiks commented Apr 3, 2024

@colmtuite

  1. Good point, will add.
  2. I'm wondering if we should use the "mouse rest" behavior instead of a delay by default? Ideally, you only want to show the tooltip when the cursor is at rest, rather than an arbitrary delay (Floating UI supports both though).
  3. Fair enough. I'm not sure, the delay being symmetric by default and is how I've always done it and there hasn't really been any complaints, but I can see why you might want it to only affect the open delay, or have separate props.
  4. This is implemented as TooltipDelayGroup in the PR currently. It can be global, or wrap only a specific group, allowing you to change the delay granularly.
  5. ^
  6. I personally like that it matches the gap prop in flexbox. But yeah, it's called offset in Floating UI - the problem is there are two different types of offsets (side and alignment axis).
  7. That does make more sense
  8. Devs may hide somewhat critical info behind tooltips in some cases, so it should be shown on touch by default imo (at least to get a consistent experience across all devices by default).

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.

@colmtuite
Copy link

  1. Is this a component? Does it exist outside of Tooltip? If it's a component, it's missing from the docs Anatomy section.
  2. sideOffset and alignmentOffset seems like the way forward here? Radix does similar.
  3. How does this work? Like what's the interaction? Touch devices don't support hover (which is how a tooltip is triggered), so I don't see how it would work?

@atomiks
Copy link
Contributor Author

atomiks commented Apr 3, 2024

  1. Yes it's a separate component, not documented currently. It's not really part of the default anatomy since it's a separate feature that wraps it. (Edit: it's now documented.)
  2. I like those! I guess separating side and alignment into separate props instead of Floating UI's placement may work better if we do that? Or is a single placement prop still more ergonomic?

  1. They tap it and it shows the tooltip; if they tap off it closes.

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 click handler rather than focus, if it's attached to a button. Not 100% sure about the screen reader/keyboard a11y of this.

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?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 3, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 4, 2024
@atomiks atomiks force-pushed the feat/Tooltip branch 4 times, most recently from 8d4d38b to 1efbbb8 Compare April 8, 2024 00:40
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Apr 8, 2024
@atomiks atomiks force-pushed the feat/Tooltip branch 3 times, most recently from 22e2843 to 11f1654 Compare April 11, 2024 06:56
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2024
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Apr 12, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 16, 2024
@atomiks atomiks force-pushed the feat/Tooltip branch 2 times, most recently from 4966245 to b3f0b25 Compare April 17, 2024 05:17
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 22, 2024
@atomiks
Copy link
Contributor Author

atomiks commented May 23, 2024

It seems that the TooltipGroup's delay is not taken into consideration.

Thanks for the spot - fixed

return (
<Tooltip.Root>
<AnchorButton>Anchor</AnchorButton>
<TooltipPopup sideOffset={7}>
Copy link
Member

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.

Copy link
Contributor Author

@atomiks atomiks May 24, 2024

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.)

docs/data/base/pages.ts Outdated Show resolved Hide resolved
packages/mui-base/src/Tooltip/Arrow/TooltipArrow.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/Tooltip/Root/TooltipRoot.tsx Outdated Show resolved Hide resolved
exiting: boolean;
};

export interface TooltipPopupProps
Copy link
Member

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)

Copy link
Contributor Author

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

@michaldudak
Copy link
Member

I just noticed the tooltip is rendered within two nested portals. Is this intended?
image

@atomiks
Copy link
Contributor Author

atomiks commented May 24, 2024

I just noticed the tooltip is rendered within two nested portals. Is this intended?

No, mistake when adding the TooltipPopupRoot component. Fixed.

Copy link
Member

@michaldudak michaldudak left a 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)

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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,
Copy link
Member

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.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 24, 2024

A quick feedback:

  1. Tooltip.Group. Should this be built-in, meaning work without a provider? I get confused seeing this component. I assumed that we would want this behavior for all tooltips on the UI, regardless of their proximity, but because using a group is painful (especially if the app is split between components) then most developers won't use it.

  2. Does most of the tests in https://github.com/mui/material-ui/blob/next/packages/mui-material/src/Tooltip/Tooltip.test.js passes? For example, are we missing mobile support?

  3. How much of https://github.com/mui/material-ui/issues?q=is%3Aopen+is%3Aissue+label%3A%22component%3A+tooltip%22 did we use to design the component? For example, [Tooltip] Tooltip touch the edge #413. I'm not asking in the sens of fixing all of these issues, but about using them to inform the design decisions, so we can later come back to the component and solve the issus with fewer breaking changes. Overall, point 2 feels a lot more important.

  4. I guess we should transfer most of the issues of 3. from the Material UI repository to here.

@oliviertassinari oliviertassinari added new feature New feature or request labels May 24, 2024
@atomiks
Copy link
Contributor Author

atomiks commented May 24, 2024

@oliviertassinari

Tooltip.Group. Should this be built-in, meaning work without a provider?

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.

Does most of the tests in https://github.com/mui/material-ui/blob/next/packages/mui-material/src/Tooltip/Tooltip.test.js passes? For example, are we missing mobile support?

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.

How much of ...

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

@colmtuite
Copy link

Tooltip.Group

  • Group/Provider should be in Anatomy. imo it should communicated as the default way of using Tooltip for almost all users.
  • For the same reason as above, Group/Provider should also be in the hero demo.

Delay

  • imo the default delay is waaaaayyy too short. I recommend 750ms as the default.
  • imo delayType default should be hover? I think it's the most common expectation, and would also match the the title attr bevaiour.

Docs Content

  • I think we should remove Infotip warning message until we merge Popover, or whatever we end up doing for Infotip. I'm not sure how/if we will handle infotips, and don't want to reference components that we don't have yet.
  • Remove “They are also useful for things like thumbnail tooltips when hovering over a progress bar when using a mouse.” just to keep the content shorter. imo you're getting the point across sufficiently without this sentence.
  • We can shorten the Placement section a good bit.
    • Merge the Placement section into a single code block.
    • Remove the mention of Tooltip.Popup. You can see where the side prop goes in the code block.
    • Remove the list of possible values? You can see this in the props table.
    • What is the use case for styling the true rendered values of placement?
  • Offsets
    • Change “Offsets” heading to “Offset”
    • Merge these into a single code block.
  • Delay
    • Merge delay and closeDelay into a single code block?
  • Remove the "Default open" docs section? It's in the API Reference and not that important.
  • Remove the "Hoverable content" docs section? It's in the API Reference.
  • Styling
    • Remove the code block?

@atomiks
Copy link
Contributor Author

atomiks commented May 27, 2024

Group/Provider should be in Anatomy. imo it should communicated as the default way of using Tooltip for almost all users.

It's a bit confusing in Anatomy since Root is the one that says "wraps all other components" but now you have this extra one. Given it's optional too, I've added it directly beneath Anatomy to explain it a bit better.

imo the default delay is waaaaayyy too short. I recommend 750ms as the default.
imo delayType default should be hover? I think it's the most common expectation, and would also match the the title attr bevaiour.

The thing with the rest type is that it can be shorter than you need with hover. With hover, the timeout starts as soon as you enter the element, even if your cursor is gliding across it unintentionally, so it needs to be longer. With rest, the user has stopped and is showing intentionality to use the tool, waiting for any extra info (the tooltip) to appear to help them. I found 200ms is about the ideal length to show the tooltip as quickly as possible without it appearing under normal clicking circumstances (i.e. the user already knows the tool by heart and doesn't want to see the tooltip appear anymore). I can make it a bit longer, but the point is it doesn't actually need to be so long with this technique.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tooltip This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bugs on Firefox
5 participants