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.

Sign up for GitHub

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

Jump to bottom

[Tooltip] Allow to interact with the tooltip #13305

Merged
merged 2 commits into from
Oct 27, 2018
Merged

[Tooltip] Allow to interact with the tooltip #13305

merged 2 commits into from
Oct 27, 2018

Conversation

jantimon
Copy link
Contributor

Hey :)

thanks for your library.

I would like to allow users to interact with the tooltip (similar to the interactive feature of react-tippy).
The current implementation will close the tooltip even if the user moved their mouse cursor above the tooltip.

This pull request will change this and add the mouseOver lister also to the tooltip.
So if you specify a leaveDelay and the user manages to move the mouse over to the tooltip within that delay it will stay open.

The change also respects the disableHoverListener and disableFocusListener options.

oliviertassinari
oliviertassinari reviewed Oct 19, 2018
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The noninteractive behavior was done on purpose. But I can understand the use case.
If we want to move forward, I think that we should put this feature behind a flag, something like
allowInteraction or interactive.

packages/material-ui/src/Tooltip/Tooltip.js Outdated
@@ -314,6 +314,10 @@ class Tooltip extends React.Component {
placement={placement}
anchorEl={this.childrenRef}
open={open}
onMouseOver={childrenProps.onMouseOver}
onMouseLeave={childrenProps.onMouseLeave}
onFocus={childrenProps.onFocus}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the focus/blur listeners?

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 would allow keyboard users to tap to a button inside the tooltip - I guess that’s rather an edge case but adding a little bit accessibility here will not cause any other side effects

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example? I can understanding the blur usage but not the focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe like the drop down nesting example here : https://atomiks.github.io/tippyjs/

Copy link
Contributor Author

@jantimon jantimon Oct 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the idea is the same - if you manage to tab over fast enough the tooltip it will stay open - if you then tab further the tooltip should close

@oliviertassinari oliviertassinari added new feature New feature or request component: tooltip This is the name of the generic UI component, not the React module! labels Oct 19, 2018
@jantimon
Copy link
Contributor Author

jantimon commented Oct 20, 2018 •
edited
Loading

I will ass a new interactive prop if you like.

Should it be like isInteractive && childrenProps.onMouseOver (and also for the other listeners)?

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 20, 2018 •
edited
Loading

@jantimon Yes, we can follow their convention:

<Tippy interactive={true}>
  <button class="btn">Interactive (hover)</button>
</Tippy>

https://github.com/atomiks/tippyjs/blob/f3714a71c7dfac24b140aeadf5237c9a63c0eaa4/website/js/sections/Demo.js#L114

@jantimon jantimon force-pushed the patch-2 branch 6 times, most recently from 5018daf to 4abee4c Compare October 21, 2018 15:24
@jantimon
Copy link
Contributor Author

@oliviertassinari thanks for your fast feedback 👍

I prepared the feature like you suggested.

@jantimon jantimon force-pushed the patch-2 branch 2 times, most recently from db4d209 to 1c528cf Compare October 23, 2018 05:31
@jantimon
Copy link
Contributor Author

@oliviertassinari All tests passed 👍

@oliviertassinari oliviertassinari self-assigned this Oct 25, 2018
oliviertassinari
oliviertassinari approved these changes Oct 27, 2018
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a great pull request!

@oliviertassinari oliviertassinari merged commit ad8695b into mui:master Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees

@oliviertassinari oliviertassinari

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jantimon @oliviertassinari