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] Unmount Tooltip on exit and mount on enter #10909

Closed
1 task done
franklixuefei opened this issue Apr 4, 2018 · 7 comments
Closed
1 task done

[Tooltip] Unmount Tooltip on exit and mount on enter #10909

franklixuefei opened this issue Apr 4, 2018 · 7 comments
Assignees
Labels
component: tooltip This is the name of the generic UI component, not the React module! new feature New feature or request
Milestone

Comments

@franklixuefei
Copy link
Contributor

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

Expected Behavior

Ideally I'd like an option to configure the Tooltip to unmountOnExit and mountOnEnter

Current Behavior

Seems like there is no easy way to achieve that.

Context

In my project, I need to place a lot of Tooltip helpers, and there are a bunch of Tooltip DOM elements that were created under body. This I think impacts the performance.

Your Environment

Tech Version
Material-UI beta.40
React 16.20
browser
etc
@franklixuefei franklixuefei changed the title [Tooltip] Unmount Tooltip on exit and mount on enter [HelpNeeded][Tooltip] Unmount Tooltip on exit and mount on enter Apr 4, 2018
@oliviertassinari oliviertassinari changed the title [HelpNeeded][Tooltip] Unmount Tooltip on exit and mount on enter [Tooltip] Unmount Tooltip on exit and mount on enter Apr 4, 2018
@oliviertassinari oliviertassinari added component: tooltip This is the name of the generic UI component, not the React module! new feature New feature or request labels Apr 4, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 4, 2018

@franklixuefei I agree, I think that it would be great to move the transition implementation of the tooltip into it's own component. It's what we do with the Modal for instance. This should allow us to solve this issue and allow people to implement custom transitions.

@dmythro
Copy link

dmythro commented May 25, 2018

I'd not suggest to unmount tooltips for Accessibility reasons. Often adding a tooltip specifies aria to be labeled by this tooltip (especially if it is an icon), so it has to be in DOM so screen reader can actually read it properly.

@franklixuefei
Copy link
Contributor Author

franklixuefei commented Jun 6, 2018

@Z-AX I didn't think of the Accessibility part. Good point. However, I've already noticed a big perf cut introduced by Tooltips not being unmounted upon disappearance, especially when they are enabled for cells in a table with lots of rows. Switching between pages has become chunky and laggy because of this. (I verified by taking out the tooltips from the table cells, and the perf dramatically boosted).
So I guess for the Accessbility part, we need to come up with something else.

@dmythro
Copy link

dmythro commented Jun 20, 2018

I've seen the Popper dependency was upgraded in v1.2.2. @franklixuefei, is performance still an issue? Just curious :) We don't have tooltips for table cells to test quickly, but overall didn't see any performance problems recently.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 20, 2018

@Z-AX It's worse with react-popper@1.x. I have reverted to v0.x. I'm might use popper.js directly in the future.

@matthiasp42
Copy link

I also had dramatic performance issues using Tooltip in a list and then scrolling. I'm assuming this is caused by Popper constantly reevaluating every items position.

I made a drop-in replacement for material-uis Tolltip that solves this issue by only rendering the Tooltip when the mouse is over the childrens element.

https://gist.github.com/matthiasp42/c561fe09f86b61e364bdd23369be5485

I'm sharing it here so it may help somebody else, but also I would love some feedback on my solution, or if this is for some reason not a proper way to do it.

@oliviertassinari
Copy link
Member

I also had dramatic performance issues using Tooltip in a list and then scrolling

Yes, it's a recent regression. It's already fixed on master. We will soon release the fix.

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

No branches or pull requests

4 participants