[Modal] Refactor tests to remove internal accesses #16262

oliviertassinari · 2019-06-17T09:20:27Z

Prepare #16254.

Replace some usage of withTheme to useTheme.
Move some default props away of .defaultProps.
Remove some dead code.
Fix a Portal bug with the ref and onRendered inversed call order.
Fix a TrapFocus bug when isEnabled is not "stable".
Repace packages/material-ui/src/ imports with @material-ui/core/.

mui-pr-bot · 2019-06-17T09:27:35Z

@material-ui/lab: parsed: -0.42% 😍, gzip: -0.31% 😍

Details of bundle changes.

Comparing: 93f5b33...44976a0

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.09%	-0.06%	319,010	318,737	87,576	87,520
@material-ui/core/Paper	-0.00%	+0.01% 🔺	68,281	68,280	20,353	20,356
@material-ui/core/Paper.esm	-0.00%	+0.02% 🔺	61,578	61,577	19,133	19,136
@material-ui/core/Popper	-0.08%	-0.06%	28,968	28,945	10,411	10,405
@material-ui/core/Textarea	0.00%	-0.04%	5,513	5,513	2,374	2,373
@material-ui/core/TrapFocus	0.00%	0.00%	3,755	3,755	1,580	1,580
@material-ui/core/styles/createMuiTheme	0.00%	+0.05% 🔺	16,012	16,012	5,791	5,794
@material-ui/core/useMediaQuery	0.00%	+0.09% 🔺	2,597	2,597	1,102	1,103
@material-ui/lab	-0.42%	-0.31%	140,580	139,991	43,454	43,319
@material-ui/styles	0.00%	+0.03% 🔺	51,703	51,703	15,337	15,342
@material-ui/system	0.00%	-0.07%	15,303	15,303	4,342	4,339
Button	-0.00%	0.00%	84,279	84,278	25,630	25,631
Modal	-0.07%	-0.37%	20,345	20,330	6,689	6,664
Slider	-0.00%	-0.03%	74,681	74,679	23,221	23,213
colorManipulator	0.00%	0.00%	3,904	3,904	1,544	1,544
docs.landing	0.00%	0.00%	55,232	55,232	13,946	13,946
docs.main	-0.02%	-0.00%	651,051	650,902	205,054	205,053
packages/material-ui/build/umd/material-ui.production.min.js	-0.07%	+0.03% 🔺	292,254	292,060	83,501	83,522

Generated by 🚫 dangerJS against 44976a0

oliviertassinari · 2019-06-17T09:26:09Z

packages/material-ui/src/Portal/Portal.js

  useEnhancedEffect(() => {
    if (onRendered && mountNode) {
      onRendered();
    }
  }, [mountNode, onRendered]);

-  React.useImperativeHandle(ref, () => mountNode || childRef.current, [mountNode]);


Change the order so ref triggers before onRendered.

Wait refs should never trigger before layout effects. Those are two different phases. Do you have a test for that?

useImperativeHandle triggers at the same time than synchronous layout effects from what I can log (not layout effects!). I'm adding a test case.

Huh, need to play around with this. Did not know that.

I believe that reason is the following: you need to have access to the DOM element in the synchronous layout effects. If the ref triggers after, you don't have it.

Did some experimenting and it is essentially facebook/react#7769 i.e. within a render block useImperativeHandle runs in the same phase as layout effects but any layout effect queued outside (e.g. in a parent) will run after useImperativeHandle of children.

I think onRendered is actually an Anti-Pattern. We already have callback refs to handle when a host component is committed. What should happen is

- const portalRef = React.useRef(); - <Portal container={container} onRendered={() => doSomethingWith(portalRef.current)} ref={portalRef} /> + <Portal container={container} ref={doSomethingWith} />

Oh, interesting. I have never thought about it

I think we can re-evaluate dropping onRendered for callback refs in v5. For now this fix works.

I'm in favor of dropping the prop in v5. I will update the Modal implementation to stop using it in the next Modal pull request. Thanks for raising. I agree that it doesn't provide anything since the forwardRef effort was completed. 👌

oliviertassinari · 2019-06-17T09:26:42Z

packages/material-ui/src/Modal/isOverflowing.js

@@ -1,10 +1,6 @@
 import ownerDocument from '../utils/ownerDocument';
 import ownerWindow from '../utils/ownerWindow';

-export function isBody(node) {


packages/material-ui/src/Modal/TrapFocus.js

oliviertassinari · 2019-06-17T09:27:47Z

packages/material-ui/src/Modal/Modal.test.js

@@ -94,52 +94,43 @@ describe('<Modal />', () => {
      const wrapper = mount(modal);
      const onClose = spy();
      wrapper.setProps({ onClose });
-
-      const handler = wrapper.find('Modal').instance().handleBackdropClick;


Remove usages of .instance().

oliviertassinari · 2019-06-17T09:28:18Z

docs/src/pages/components/modal/SimpleModal.tsx

+  }),
+);
+
+export default function SimpleModal() {


Add a TypeScript demo 👌

oliviertassinari · 2019-06-17T09:29:05Z

packages/material-ui/test/integration/fixtures/select/SelectAndDialog.js

-import Dialog from 'packages/material-ui/src/Dialog';
+import MenuItem from '@material-ui/core/MenuItem';
+import Select from '@material-ui/core/Select';
+import Dialog from '@material-ui/core/Dialog';


I have removed all the usages of imports from packages/. Better have fewer ways of doing the same thing!

oliviertassinari · 2019-06-17T09:30:31Z

packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js

 };

-export default withTheme(SwipeableDrawer);
+export default SwipeableDrawer;


Remove most of the usages of withTheme(). It should help with perf, bundle size and debug noise.

oliviertassinari · 2019-06-17T09:31:41Z

packages/material-ui/src/Portal/Portal.js

@@ -69,10 +69,6 @@ Portal.propTypes = {
  onRendered: PropTypes.func,
 };

-Portal.defaultProps = {


Move some of the .defaultProps to prop destructuring. I haven't migrated everything. I have left the complex objects and the one used in the custom prop-types.

packages/material-ui/src/Modal/TrapFocus.js

packages/material-ui/src/Popover/Popover.js

eps1lon · 2019-06-17T12:03:42Z

packages/material-ui/src/Portal/Portal.js

  useEnhancedEffect(() => {
    if (onRendered && mountNode) {
      onRendered();
    }
  }, [mountNode, onRendered]);

-  React.useImperativeHandle(ref, () => mountNode || childRef.current, [mountNode]);


Wait refs should never trigger before layout effects. Those are two different phases. Do you have a test for that?

updated

oliviertassinari added test component: modal This is the name of the generic UI component, not the React module! labels Jun 17, 2019

oliviertassinari commented Jun 17, 2019

View reviewed changes

oliviertassinari mentioned this pull request Jun 17, 2019

[Modal] Migrate to hooks #16254

Merged

eps1lon previously requested changes Jun 17, 2019

View reviewed changes

oliviertassinari added 3 commits June 17, 2019 22:14

[Modal] Refactor tests to remove internal accesses

f55cb1b

[Portal] Test that rendered is called after the ref

2cb38a9

reduce default prop fragmentation

7724920

oliviertassinari force-pushed the modal-e2e-tests branch from 4756d1f to 89dde00 Compare June 17, 2019 21:21

Prepare migration to react-dom/FocusScope

69420df

oliviertassinari force-pushed the modal-e2e-tests branch 2 times, most recently from cf0664f to 6ebe7f6 Compare June 18, 2019 10:31

revert trap focus changes

44976a0

oliviertassinari force-pushed the modal-e2e-tests branch from 6ebe7f6 to 44976a0 Compare June 18, 2019 11:02

oliviertassinari requested a review from joshwooding June 18, 2019 15:39

oliviertassinari added the PR: good for merge label Jun 18, 2019

eps1lon merged commit 7bcf033 into mui:master Jun 18, 2019

oliviertassinari deleted the modal-e2e-tests branch June 18, 2019 21:05

oliviertassinari mentioned this pull request Jul 21, 2019

[Portal] Prepare deprecation of onRendered #16597

Merged

oliviertassinari mentioned this pull request Mar 6, 2020

[RFC] Material-UI v5 🚀 #20012

Closed

eps1lon mentioned this pull request Sep 3, 2020

[core] Remove onRendered from Modal and Portal #22464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Modal] Refactor tests to remove internal accesses #16262

[Modal] Refactor tests to remove internal accesses #16262

oliviertassinari commented Jun 17, 2019 •

edited

Loading

mui-pr-bot commented Jun 17, 2019 •

edited

Loading

oliviertassinari Jun 17, 2019 •

edited

Loading

eps1lon Jun 17, 2019

oliviertassinari Jun 17, 2019

eps1lon Jun 18, 2019

oliviertassinari Jun 18, 2019

eps1lon Jun 18, 2019

eps1lon Jun 18, 2019

oliviertassinari Jun 18, 2019

eps1lon Jun 18, 2019

oliviertassinari Jun 18, 2019

oliviertassinari Jun 17, 2019

oliviertassinari Jun 17, 2019

oliviertassinari Jun 17, 2019

oliviertassinari Jun 17, 2019

oliviertassinari Jun 17, 2019

oliviertassinari Jun 17, 2019

eps1lon Jun 17, 2019

[Modal] Refactor tests to remove internal accesses #16262

[Modal] Refactor tests to remove internal accesses #16262

Conversation

oliviertassinari commented Jun 17, 2019 • edited Loading

mui-pr-bot commented Jun 17, 2019 • edited Loading

oliviertassinari Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jun 17, 2019 •

edited

Loading

mui-pr-bot commented Jun 17, 2019 •

edited

Loading

oliviertassinari Jun 17, 2019 •

edited

Loading