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] Add disabled prop #21204

Closed
1 task done
DaniGuardiola opened this issue May 25, 2020 · 14 comments
Closed
1 task done

[Tooltip] Add disabled prop #21204

DaniGuardiola opened this issue May 25, 2020 · 14 comments
Labels
component: tooltip This is the name of the generic UI component, not the React module! waiting for 👍 Waiting for upvotes

Comments

@DaniGuardiola
Copy link

DaniGuardiola commented May 25, 2020

  • I have searched the issues of this repository and believe that this is not a duplicate.

I found this issue which is vaguely related, I think: #1500

Summary 💡

Sometimes it's useful to have an uncontrolled tooltip for, say, an icon button that opens a menu. When the menu is open, the tooltip might overlap with the menu, so it's best to hide it.

Now, you can hide the tooltip by setting the title to an empty string, but this has the issue that the tooltip suddenly collapses while fading away, which is not very nice. I've tried other ways like disabling the listeners but that causes a lot of problems.

Making the tooltip controlled and manually implementing the event listeners and all is overkill for just this thing.

I think it could be easily solved with a disabled property, so that it can be safely disabled without this ugly UI issue while remaining uncontrolled.

Examples 🌈

Instead of:

<Tooltip title={menuOpen ? '' : tooltip} />

Do:

<Tooltip disabled={menuOpen} />

Motivation 🔦

I can share the app I'm building with you if you need more details of a real use-case. Just ask.

@oliviertassinari oliviertassinari added the component: tooltip This is the name of the generic UI component, not the React module! label May 25, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented May 25, 2020

@DaniGuardiola It would have been great to provide a live codesandbox example of the problem to illustrate the menu issue you are referring to. But basically, same as https://codepen.io/oliviertassinari/pen/mdegLMa.

Capture d’écran 2020-05-28 à 10 54 04

It's something we can face frequently on icon buttons, agreed, I have seed it on Gmail for instance.

Capture d’écran 2020-05-28 à 10 55 27

I think that it would help to show how you would use the Tooltip with and without this prop. It will help better evaluate the pros. I'm personally in favor of the proposal.

@oliviertassinari oliviertassinari added the new feature New feature or request label May 25, 2020
@oliviertassinari oliviertassinari changed the title Disable uncontrolled tooltip [Tooltip] Add disabled prop May 25, 2020
@DaniGuardiola
Copy link
Author

DaniGuardiola commented May 26, 2020

@oliviertassinari I can try to make a codesandbox if I find any spare time soon. In the meantime, check my (WIP and broken) text editor at lab.daniguardiola.me

The menu to the left has two elements which display a sub-menu with this problem. Hope that can be enough for now until I can get around to coding the codesandbox.

Thank you :)

@eps1lon
Copy link
Member

eps1lon commented May 28, 2020

I couldn't reproduce this issue in any of the links. It's not obvious to me that the issue description isn't a bug. A tooltip should not appear if a menu is open.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 28, 2020

@eps1lon I have updated my initial comment to contain screenshots, to make it clearer to the others. I'm confused, are you saying that we should label this issue a bug and not an enhancement?

@oliviertassinari
Copy link
Member

Hum actually, I couldn't reproduce: https://codesandbox.io/s/material-demo-sdgqw?file=/demo.js. We might want to ignore the problem and close the issue.

@oliviertassinari oliviertassinari added status: waiting for author Issue with insufficient information and removed new feature New feature or request labels May 28, 2020
@eps1lon
Copy link
Member

eps1lon commented May 28, 2020

I meant that I do understand what you're seeing but not how we can reproduce this with Material-UI.

@oliviertassinari
Copy link
Member

@DaniGuardiola Do you have a reproduction?

@DaniGuardiola
Copy link
Author

DaniGuardiola commented May 28, 2020

@eps1lon @oliviertassinari

Hi, yes. I'm not talking about a Material-UI menu. I'm talking about a custom menu I created and that I control. That's the case in which the tooltip should remain closed. Actually this is not the point, just realized. The point is that I want the tooltip to be uncontrolled. The codesandbox provided does fix the issue but it does so with a controlled tooltip, which is what I would like to avoid, as it becomes too cumbersome just for this little issue.

Check out my ongoing project. If you click the lower button of the left menu you'll see this behavior.

Let me try to create a codesandbox as well.

@DaniGuardiola
Copy link
Author

DaniGuardiola commented May 28, 2020

Here's a reproduction of the issue: https://codesandbox.io/s/material-demo-wcfo1?file=/demo.js

Here's my current solution (check the 'title' prop of the tooltip): https://codesandbox.io/s/material-demo-v67ss?file=/demo.js

Here's what I propose: https://codesandbox.io/s/material-demo-kcuko?file=/demo.js

BTW, just noticed that this seems to disable the inner IconButton. Is this intended behavior? It confuses me.

@DaniGuardiola
Copy link
Author

Oh and I noticed something by looking at your last codesandbox @oliviertassinari

I noticed that you're using a controlled tooltip, but using the onClose and onOpen events. While this is a way to achieve what I need that I didn't know existed, it seems to me like the addition of a disabled prop would still make my case simpler to fix, as the user (in this case me) doesn't have to worry about controlling every tooltip but still has a way to override the behavior.

@DaniGuardiola
Copy link
Author

Updated this comment, as I just realized I was wrong.

@oliviertassinari
Copy link
Member

@DaniGuardiola Thanks for the extra details and the discussion. I think that we can close, looking at your application, you shouldn't even need to make the title an empty string. Also, it's not a frequent problem, hence the controllable approach sounds fair.

@oliviertassinari oliviertassinari added waiting for 👍 Waiting for upvotes and removed status: waiting for author Issue with insufficient information labels May 28, 2020
@DaniGuardiola
Copy link
Author

Okay, well in case anybody has this same issue, here's a workaround:

import React, { useState, useEffect } from 'react'

import { Tooltip } from '@material-ui/core'

function DisableableTooltip ({
  disabled,
  children,
  ...tooltipProps
}: TooltipProps & { disabled: boolean }) {
  const [open, setOpen] = useState(false)

  useEffect(() => {
    if (disabled) setOpen(false)
  }, [disabled])

  return (
    <Tooltip
      {...tooltipProps}
      open={open}
      onOpen={() => !disabled && setOpen(true)}
      onClose={() => setOpen(false)}
    >
      {children}
    </Tooltip>
  )
}

Use this component as a drop-in replacement of Toolbar with the additional disabled property.

@Ethorsen
Copy link

Thanks, needed this fix for the exact same issue.

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! waiting for 👍 Waiting for upvotes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants