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

[Transition] Fix false-positive ref warning #15526

Merged
merged 4 commits into from Apr 30, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 30, 2019

Fix false-positive ref warning with suggestion from @oliviertassinari
Closes #15515

useForkRef always created a callback even if none of the branches required a ref leading to false positives when the component didn't need the ref but just forwarded the ref to a clones child.

Used this opportunity to improve tests for useForkRef. Especially ref cleanup was never actually tested.

@eps1lon eps1lon added bug 🐛 Something doesn't work component: transitions This is the name of the generic UI component, not the React module! labels Apr 30, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 30, 2019

Details of bundle changes.

Comparing: 8c5a1ca...108456c

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 +0.02% 🔺 311,394 311,431 84,601 84,619
@material-ui/core/Paper 0.00% 0.00% 67,623 67,623 20,124 20,124
@material-ui/core/Paper.esm 0.00% 0.00% 60,988 60,988 19,020 19,020
@material-ui/core/Popper +0.12% 🔺 +0.10% 🔺 31,114 31,151 10,803 10,814
@material-ui/core/Textarea +0.68% 🔺 +0.72% 🔺 5,468 5,505 2,365 2,382
@material-ui/core/TrapFocus +0.99% 🔺 +0.58% 🔺 3,731 3,768 1,565 1,574
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,943 15,943 5,777 5,777
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab +0.03% 🔺 +0.04% 🔺 140,997 141,034 42,651 42,666
@material-ui/styles 0.00% 0.00% 51,151 51,151 15,148 15,148
@material-ui/system 0.00% 0.00% 11,765 11,765 3,923 3,923
Button +0.04% 🔺 +0.06% 🔺 85,901 85,938 25,732 25,747
Modal +0.18% 🔺 +0.14% 🔺 20,575 20,612 6,603 6,612
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,486 51,486 11,342 11,342
docs.main +0.01% 🔺 +0.01% 🔺 648,595 648,632 202,362 202,378
packages/material-ui/build/umd/material-ui.production.min.js +0.01% 🔺 +0.01% 🔺 293,109 293,146 82,525 82,535

Generated by 🚫 dangerJS against 108456c

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.

👌

@oliviertassinari oliviertassinari merged commit 7239977 into mui:next Apr 30, 2019
@eps1lon eps1lon deleted the fix/useForkRef/unnecessary-ref branch April 30, 2019 14:34
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: transitions 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.

[Zoom] Not accepting child component
3 participants