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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tooltip] Rework the implementation #12085

Merged
merged 1 commit into from Jul 8, 2018

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jul 8, 2018

I'm sorry, I should have done smaller pull requests fixing one issue after another, but it was so much fun! Yes, it's closing 20 issues. Hopefully, it's not introducing any breaking changes.


The implementations I have been benchmarking before / during the effort:

@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 Jul 8, 2018
@oliviertassinari oliviertassinari force-pushed the tooltip-v2 branch 3 times, most recently from 8fbd01d to 9ccb79d Compare July 8, 2018 18:40
@@ -27,13 +27,13 @@ module.exports = [
name: 'The size of all the modules of material-ui.',
webpack: true,
path: 'packages/material-ui/build/index.js',
limit: '95.3 KB',
limit: '94.3 KB',
Copy link
Member Author

Choose a reason for hiding this comment

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

馃殌

@dmythro
Copy link

dmythro commented Jul 9, 2018

@oliviertassinari, regarding #10909. Tooltips are used for Accessibility only when mounted (aria-labeledby). Are you sure it doesn't break anything without mounted ones?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 9, 2018

@Z-AX If people are providing a string title property, we apply it to the children when the tooltip is closed. This is the nominal use case. We are good.
What if people are providing something else? We have the aria-labeledby. Let me know if you think we need something else for a11y.

@mbrookes
Copy link
Member

mbrookes commented Jul 9, 2018

@oliviertassinari Awesome job!

@dmythro
Copy link

dmythro commented Jul 9, 2018

Thanks! We'll check this out within the next sprint.

@oliviertassinari oliviertassinari mentioned this pull request Jul 11, 2018
1 task
@bcolloran
Copy link

Rad, this looks like exactly what my team needs for some perf issues we've been trying to get to the bottom of. I'm not familiar with your release process -- any idea roughly how long it might be until this patch is rolled into a release and available on NPM? I want to be sure to update when it's ready!

Thanks for your work on this! 馃憤

@oliviertassinari
Copy link
Member Author

I need it too for the office. We have a critical screen that surfers from an important performance issue.
It will be release this weekend. I didn't want to release it right away. It's a major refacto. It's safer this way.

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.

None yet

4 participants