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

[core] Use useFormControl instead of withFormControlState #16503

Merged
merged 5 commits into from
Jul 7, 2019
Merged

[core] Use useFormControl instead of withFormControlState #16503

merged 5 commits into from
Jul 7, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 6, 2019

  • replace private hoc with public hook
  • rewrote tests to work without usage of internal FormControlContext.Provider.

@eps1lon eps1lon added the package: material-ui Specific to @mui/material label Jul 6, 2019
eps1lon
eps1lon commented Jul 6, 2019
View reviewed changes
packages/material-ui/src/FormControl/FormControl.test.js
@@ -105,9 +105,7 @@ describe('<FormControl />', () => {
);
expect(readContext.args[0][0]).to.have.property('focused', false);

act(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

unnecessary act

eps1lon
eps1lon commented Jul 6, 2019
View reviewed changes
packages/material-ui/src/FormControlLabel/FormControlLabel.test.js
@@ -30,126 +35,141 @@ describe('<FormControlLabel />', () => {
}));

it('should render the label text inside an additional element', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test never tested that there is an additional element. Now it does.

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 6, 2019 •
edited
Loading

@material-ui/core: parsed: +Infinity% , gzip: +Infinity%
@material-ui/lab: parsed: +Infinity% , gzip: +Infinity%
@material-ui/styles: parsed: +Infinity% , gzip: +Infinity%
@material-ui/system: parsed: +Infinity% , gzip: +Infinity%

Details of bundle changes.

Comparing: cf2cdd8...d933cd0

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +Infinity% 🔺 +Infinity% 🔺 0 327,350 0 90,371
@material-ui/core/Paper +Infinity% 🔺 +Infinity% 🔺 0 68,291 0 20,368
@material-ui/core/Paper.esm +Infinity% 🔺 +Infinity% 🔺 0 61,574 0 19,156
@material-ui/core/Popper +Infinity% 🔺 +Infinity% 🔺 0 28,942 0 10,407
@material-ui/core/Textarea +Infinity% 🔺 +Infinity% 🔺 0 5,505 0 2,362
@material-ui/core/TrapFocus +Infinity% 🔺 +Infinity% 🔺 0 3,753 0 1,577
@material-ui/core/styles/createMuiTheme +Infinity% 🔺 +Infinity% 🔺 0 16,012 0 5,790
@material-ui/core/useMediaQuery +Infinity% 🔺 +Infinity% 🔺 0 2,595 0 1,103
@material-ui/lab +Infinity% 🔺 +Infinity% 🔺 0 140,004 0 43,416
@material-ui/styles +Infinity% 🔺 +Infinity% 🔺 0 51,699 0 15,347
@material-ui/system +Infinity% 🔺 +Infinity% 🔺 0 15,428 0 4,396
Button +Infinity% 🔺 +Infinity% 🔺 0 84,442 0 25,742
Modal +Infinity% 🔺 +Infinity% 🔺 0 14,515 0 5,095
Portal +Infinity% 🔺 +Infinity% 🔺 0 3,471 0 1,573
Slider +Infinity% 🔺 +Infinity% 🔺 0 74,976 0 23,295
colorManipulator +Infinity% 🔺 +Infinity% 🔺 0 3,904 0 1,543
docs.landing +Infinity% 🔺 +Infinity% 🔺 0 54,338 0 13,762
docs.main +Infinity% 🔺 +Infinity% 🔺 0 647,009 0 203,904
packages/material-ui/build/umd/material-ui.production.min.js +Infinity% 🔺 +Infinity% 🔺 0 299,721 0 85,971

Generated by 🚫 dangerJS against d933cd0

@eps1lon eps1lon marked this pull request as ready for review July 6, 2019 11:36
oliviertassinari
oliviertassinari reviewed Jul 6, 2019
View reviewed changes
packages/material-ui/src/internal/SwitchBase.test.js
});
expect(handleChange.callCount).to.equal(1);
// event.target.check is true
expect(handleChange.firstCall.returnValue).to.be.true;
Copy link
Member

@oliviertassinari oliviertassinari Jul 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What's the advantage over using the generic equal?

Suggested change
expect(handleChange.firstCall.returnValue).to.be.true;
expect(handleChange.firstCall.returnValue).to.equal(true);

For reference, I can't find this API equivalent in the Jest documentation: https://jestjs.io/.

Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari
https://jestjs.io/docs/en/expect.html#toequalvalue
https://jestjs.io/docs/en/expect.html#tobevalue

Copy link
Member

Choose a reason for hiding this comment

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

@merceyz Thanks for the links. Do you know if there is a .toBeTrue() like API?

Copy link
Member

Choose a reason for hiding this comment

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

https://jestjs.io/docs/en/expect.html#tobetruthy

That's the closest you'll get

Copy link
Member

Choose a reason for hiding this comment

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

@eps1lon What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not following what the issue is? This appeared to be a question about jest which was answered

Copy link
Member

@oliviertassinari oliviertassinari Jul 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

My proposal is to replace the usages of to.be.true and to.be.false with the generic to.be.equal API. I have used Jest's API as a benchmark. The opportunity is to reduce the API surface.

@eps1lon eps1lon merged commit 673caa4 into mui:master Jul 7, 2019
@eps1lon eps1lon deleted the test/formControlState branch July 7, 2019 11:19
@joshwooding joshwooding mentioned this pull request Jul 7, 2019
Closed
29 tasks
@oliviertassinari oliviertassinari mentioned this pull request Feb 10, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@merceyz merceyz merceyz left review comments

@joshwooding joshwooding joshwooding left review comments

Assignees
No one assigned
Labels
package: material-ui Specific to @mui/material
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eps1lon @mui-pr-bot @oliviertassinari @merceyz @joshwooding