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

[Portal] Fix disablePortal not working #15701

Merged
merged 6 commits into from
May 15, 2019
Merged

[Portal] Fix disablePortal not working #15701

merged 6 commits into from
May 15, 2019

Conversation

paperclover
Copy link

@paperclover paperclover commented May 14, 2019

Applies the change shown here to fix issue #15612

Closes #15612.

@mui-pr-bot
Copy link

mui-pr-bot commented May 14, 2019

Details of bundle changes.

Comparing: 014a07f...4fc3681

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 317,580 317,582 86,695 86,696
@material-ui/core/Paper 0.00% 0.00% 67,869 67,869 20,171 20,171
@material-ui/core/Paper.esm 0.00% 0.00% 61,152 61,152 18,956 18,956
@material-ui/core/Popper +0.01% 🔺 +0.01% 🔺 28,738 28,740 10,351 10,352
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,384 2,384
@material-ui/core/TrapFocus 0.00% 0.00% 3,744 3,744 1,580 1,580
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,960 15,960 5,779 5,779
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% 0.00% 140,516 140,518 42,852 42,854
@material-ui/styles 0.00% 0.00% 51,353 51,353 15,188 15,188
@material-ui/system 0.00% 0.00% 14,458 14,458 4,175 4,175
Button 0.00% 0.00% 85,781 85,781 25,769 25,769
Modal +0.01% 🔺 +0.01% 🔺 20,342 20,344 6,699 6,700
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 47,836 47,836 10,920 10,920
docs.main 0.00% 0.00% 657,133 657,135 206,077 206,081
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 296,491 296,493 84,085 84,086

Generated by 🚫 dangerJS against 4fc3681

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.

.

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.

Would be nice if we could include a test that would've failed without this fix.

@eps1lon eps1lon added this to the v4 milestone May 14, 2019
@eps1lon eps1lon added bug 🐛 Something doesn't work component: Portal The React component. labels May 14, 2019
@oliviertassinari oliviertassinari self-assigned this May 15, 2019
@oliviertassinari oliviertassinari removed their assignment May 15, 2019
@oliviertassinari
Copy link
Member

I have added a failing test case, removed the class api, make the test strict mode ready and remove the outdated portal mock. This needed some proper clean up :).

@oliviertassinari oliviertassinari requested a review from eps1lon May 15, 2019 12:32
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.

Very clean test. Just the disablePortal behavior needs a better description.

@oliviertassinari oliviertassinari dismissed eps1lon’s stale review May 15, 2019 14:58

Thanks, I have removed the disablePortal test suite. I think that it was redundant.

@@ -16,8 +16,7 @@ describe('<Popper />', () => {
};

before(() => {
// StrictModeViolation: uses Portal
mount = createMount({ strict: false });
mount = createMount({ strict: true });
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@oliviertassinari oliviertassinari merged commit 28eac11 into mui:next May 15, 2019
@paperclover paperclover deleted the #15612 branch May 16, 2019 19:21
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: Portal The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Portal doesn't work when disablePortal set to true on mount
4 participants