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.

Sign up for GitHub

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

Jump to bottom

[test] Remove test-only class wrappers for higher-order components #15017

Merged
merged 2 commits into from Mar 23, 2019
Merged

[test] Remove test-only class wrappers for higher-order components #15017

merged 2 commits into from Mar 23, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Mar 22, 2019 •
edited

Class wrappers for certain higher-order components were required before facebook/react#15100.

There is still a bug in enzyme that prevents state() usage but instance().state should be equivalent. state() probably has some additional guards for older component patterns.

@eps1lon eps1lon added test package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Mar 22, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 22, 2019 •
edited

@material-ui/styles: parsed: -2.36% 😍, gzip: -1.83% 😍

Details of bundle changes.

Comparing: f9896bc...0e661ba

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.02% -0.01% 352,709 352,651 90,549 90,539
@material-ui/core/Paper -1.81% -1.32% 68,626 67,382 19,973 19,709
@material-ui/core/Paper.esm -1.95% -1.44% 62,358 61,143 19,073 18,799
@material-ui/core/Popper 0.00% -0.01% 30,460 30,460 10,528 10,527
@material-ui/core/styles/createMuiTheme 0.00% +0.02% 🔺 17,384 17,384 5,726 5,727
@material-ui/core/useMediaQuery 0.00% 0.00% 2,469 2,469 1,038 1,038
@material-ui/lab -0.04% -0.03% 152,181 152,123 44,528 44,513
@material-ui/styles -2.36% -1.83% 53,837 52,565 15,615 15,330
@material-ui/system 0.00% -0.07% 17,138 17,138 4,522 4,519
Button -0.03% -0.18% 90,918 90,889 26,969 26,921
Modal -0.07% -0.08% 84,712 84,654 25,262 25,241
colorManipulator 0.00% 0.00% 3,232 3,232 1,300 1,300
docs.landing 0.00% 0.00% 51,843 51,843 11,349 11,349
docs.main -0.02% -0.08% 647,785 647,637 201,132 200,966
packages/material-ui/build/umd/material-ui.production.min.js 0.00% +0.01% 🔺 301,799 301,799 83,742 83,754

Generated by 🚫 dangerJS against 0e661ba

@eps1lon
[MenuList] Adjust clientHeight test
0e661ba 
Don't know why the bounding box of
the ref element changed in the browser tests
oliviertassinari
oliviertassinari approved these changes Mar 22, 2019
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited

Choose a reason for hiding this comment

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

Awesome, does it mean we can remove the custom withTheme implementation too?

https://github.com/mui-org/material-ui/blob/f9896bcd129a1209153106296b3d2487547ba205/packages/material-ui-styles/src/withTheme/withTheme.js#L26-L35

@eps1lon eps1lon changed the title [test] Use actual withStyles implementation [test] Remove test-only class wrappers for higher-order components Mar 23, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Mar 23, 2019

Awesome, does it mean we can remove the custom withTheme implementation too?

Should be covered. Description was just incomplete.

eps1lon
eps1lon commented Mar 23, 2019
View reviewed changes
packages/material-ui/src/MenuList/MenuList.test.js
@@ -72,7 +72,7 @@ describe('<MenuList />', () => {
assert.strictEqual(list.style.paddingLeft, '');
assert.strictEqual(list.style.width, '');
menuListActionsRef.current.adjustStyleForScrollbar(
{ clientHeight: 10 },
{ clientHeight: 20 },
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @ryancogswell

Not entirely sure why. I think it has something to do with ref now being attached to the actual host component instead of the class wrapper. Without this change browser test were failing: https://circleci.com/gh/mui-org/material-ui/79900

Copy link
Member

Choose a reason for hiding this comment

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

Prior to the change, no CSS was injected in the page, now, it is. I have already seen this behavior in the past. The newly injected CSS change the layout values, now, they are correct.
The new styles come from the List component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes the change even better I suppose.

@oliviertassinari oliviertassinari merged commit 1d91e0a into mui:next Mar 23, 2019
@joshwooding joshwooding mentioned this pull request Mar 23, 2019
Merged
1 task
@oliviertassinari
Copy link
Member

I merge so we can move #14965 (comment) and #14958 forward.

@eps1lon eps1lon deleted the test/remove-with-styles-test branch April 16, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eps1lon @mui-pr-bot @oliviertassinari