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] Fix excess spacing #23233

Merged
merged 2 commits into from
Oct 26, 2020
Merged

[Tooltip] Fix excess spacing #23233

merged 2 commits into from
Oct 26, 2020

Conversation

benneq
Copy link
Contributor

@benneq benneq commented Oct 24, 2020

This fixed to Tooltip hover. Now the mouse can move between Button and Tooltip, but the Tooltip disappears when mouse cursor is beyond the Tooltip

Related to #23232 (comment)

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

We need the margin in both directions because it can swap direction (when there isn't enough space).

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 24, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 77a3fba

@benneq
Copy link
Contributor Author

benneq commented Oct 24, 2020 via email

@oliviertassinari
Copy link
Member

I hoped that Tooltip would then switch the CSS class to the top version, which then has the margin on the opposite (= correct) site

We didn't implement it this way, but we could consider it. It might be a bit harder to customize but that could mitigate the issue we have. Still, I think that we should get @eps1lon if your change is OK from an a11y point of view.

@oliviertassinari oliviertassinari added the component: tooltip This is the name of the generic UI component, not the React module! label Oct 24, 2020
@benneq
Copy link
Contributor Author

benneq commented Oct 25, 2020

It might be a bit harder to customize but that could mitigate the issue we have.

I played around a bit with the code, and it seems it already works like expected:

https://github.com/mui-org/material-ui/blob/4fa3f7bc30da1c86bb5702df06dbcff61c538e4b/packages/material-ui/src/Tooltip/Tooltip.js#L563
placementInner automatically switches from bottom to top if the button is at the bottom of the screen. Though it automatically applies tooltipPlacementTop when position = 'bottom'.

Or did I miss something?

@oliviertassinari oliviertassinari changed the title Fix Tooltip hover excess [Tooltip] Fix hover excess Oct 25, 2020
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Oct 25, 2020
@eps1lon eps1lon changed the title [Tooltip] Fix hover excess [Tooltip] Remove excess spacing Oct 26, 2020
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Good catch 👍 Should be fine since we still have padding and with a magnifier it should have sufficient hit area.

Aside: The distance should probably be customizable anyway. Seems to me that in certain UIs the tooltip is too far away e.g. in the top toolbar (with the search) of https://deploy-preview-23233--material-ui.netlify.app/components/tooltips/#main-content there's no real reason to move it 14px away (which doesn't even align with the grid).

@oliviertassinari oliviertassinari merged commit c7957b3 into mui:next Oct 26, 2020
@oliviertassinari oliviertassinari changed the title [Tooltip] Remove excess spacing [Tooltip] Fix excess spacing Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants