From eef55cf8a13d901518195c8f0e1287483dfd3ee0 Mon Sep 17 00:00:00 2001 From: luffy-wang <249644014@qq.com> Date: Fri, 6 Dec 2019 16:28:04 +0800 Subject: [PATCH 1/4] [Portal] fix the children handle for null or undefined --- packages/material-ui/src/Portal/Portal.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/material-ui/src/Portal/Portal.js b/packages/material-ui/src/Portal/Portal.js index 88dceb978a62cb..67b8a8a9a6af41 100644 --- a/packages/material-ui/src/Portal/Portal.js +++ b/packages/material-ui/src/Portal/Portal.js @@ -20,7 +20,7 @@ const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect const Portal = React.forwardRef(function Portal(props, ref) { const { children, container, disablePortal = false, onRendered } = props; const [mountNode, setMountNode] = React.useState(null); - const handleRef = useForkRef(children.ref, ref); + const handleRef = useForkRef(React.isValidElement(children) ? children.ref : null, ref); useEnhancedEffect(() => { if (!disablePortal) { @@ -46,10 +46,12 @@ const Portal = React.forwardRef(function Portal(props, ref) { }, [onRendered, mountNode, disablePortal]); if (disablePortal) { - React.Children.only(children); - return React.cloneElement(children, { - ref: handleRef, - }); + if (React.isValidElement(children)) { + return React.cloneElement(children, { + ref: handleRef, + }); + } + return children; } return mountNode ? ReactDOM.createPortal(children, mountNode) : mountNode; From 836480528489097105ce06560144a8295626b69e Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Thu, 5 Dec 2019 12:01:50 +0100 Subject: [PATCH 2/4] add a test case --- .../material-ui/src/Portal/Portal.test.js | 168 +++++++++--------- 1 file changed, 87 insertions(+), 81 deletions(-) diff --git a/packages/material-ui/src/Portal/Portal.test.js b/packages/material-ui/src/Portal/Portal.test.js index 2c903303f16685..0825b1a6347cee 100644 --- a/packages/material-ui/src/Portal/Portal.test.js +++ b/packages/material-ui/src/Portal/Portal.test.js @@ -1,22 +1,18 @@ /* eslint-disable react/prop-types */ import React from 'react'; -import { assert } from 'chai'; +import { expect } from 'chai'; import { spy } from 'sinon'; -import { createMount, createRender } from '@material-ui/core/test-utils'; +import { createRender } from '@material-ui/core/test-utils'; import consoleErrorMock from 'test/utils/consoleErrorMock'; +import { createClientRender } from 'test/utils/createClientRender'; import Portal from './Portal'; describe('', () => { - let mount; - let render; + let serverRender; + const render = createClientRender({ strict: true }); before(() => { - mount = createMount({ strict: true }); - render = createRender(); - }); - - after(() => { - mount.cleanUp(); + serverRender = createRender(); }); describe('server-side', () => { @@ -34,64 +30,65 @@ describe('', () => { }); it('render nothing on the server', () => { - const markup1 = render(
Bar
); - assert.strictEqual(markup1.text(), 'Bar'); + const markup1 = serverRender(
Bar
); + expect(markup1.text()).to.equal('Bar'); - const markup2 = render( + const markup2 = serverRender(
Bar
, ); - assert.strictEqual(markup2.text(), ''); + expect(markup2.text()).to.equal(''); }); }); describe('ref', () => { it('should have access to the mountNode when disabledPortal={false}', () => { const refSpy = spy(); - const wrapper = mount( + const { unmount } = render(

Foo

, ); - assert.deepEqual(refSpy.args, [[document.body]]); - wrapper.unmount(); - assert.deepEqual(refSpy.args, [[document.body], [null]]); + expect(refSpy.args).to.deep.equal([[document.body]]); + unmount(); + expect(refSpy.args).to.deep.equal([[document.body], [null]]); }); it('should have access to the mountNode when disabledPortal={true}', () => { const refSpy = spy(); - const wrapper = mount( + const { unmount } = render(

Foo

, ); const mountNode = document.querySelector('.woofPortal'); - assert.deepEqual(refSpy.args, [[mountNode]]); - wrapper.unmount(); - assert.deepEqual(refSpy.args, [[mountNode], [null]]); + expect(refSpy.args).to.deep.equal([[mountNode]]); + unmount(); + expect(refSpy.args).to.deep.equal([[mountNode], [null]]); }); it('should have access to the mountNode when switching disabledPortal', () => { const refSpy = spy(); - const wrapper = mount( + const { setProps, unmount } = render(

Foo

, ); const mountNode = document.querySelector('.woofPortal'); - assert.deepEqual(refSpy.args, [[mountNode]]); - wrapper.setProps({ + expect(refSpy.args).to.deep.equal([[mountNode]]); + setProps({ disablePortal: false, + ref: refSpy, }); - assert.deepEqual(refSpy.args, [[mountNode], [null], [document.body]]); - wrapper.unmount(); - assert.deepEqual(refSpy.args, [[mountNode], [null], [document.body], [null]]); + expect(refSpy.args).to.deep.equal([[mountNode], [null], [document.body]]); + unmount(); + expect(refSpy.args).to.deep.equal([[mountNode], [null], [document.body], [null]]); }); }); it('should render in a different node', () => { - mount( + render(

Foo

@@ -100,8 +97,8 @@ describe('', () => {
, ); const rootElement = document.querySelector('#test1'); - assert.strictEqual(rootElement.contains(document.querySelector('.woofPortal1')), true); - assert.strictEqual(rootElement.contains(document.querySelector('.woofPortal2')), false); + expect(rootElement.contains(document.querySelector('.woofPortal1'))).to.equal(true); + expect(rootElement.contains(document.querySelector('.woofPortal2'))).to.equal(false); }); it('should unmount when parent unmounts', () => { @@ -122,29 +119,29 @@ describe('', () => { ); } - const wrapper = mount(); - assert.strictEqual(document.querySelectorAll('#test1').length, 1); - wrapper.setProps({ show: false }); - assert.strictEqual(document.querySelectorAll('#test1').length, 0); + const { setProps } = render(); + expect(document.querySelectorAll('#test1').length).to.equal(1); + setProps({ show: false }); + expect(document.querySelectorAll('#test1').length).to.equal(0); }); it('should render overlay into container (document)', () => { - mount( + render(
, ); - assert.strictEqual(document.querySelectorAll('#test2').length, 1); + expect(document.querySelectorAll('#test2').length).to.equal(1); }); it('should render overlay into container (DOMNode)', () => { const container = document.createElement('div'); - mount( + render(
, ); - assert.strictEqual(container.querySelectorAll('#test2').length, 1); + expect(container.querySelectorAll('#test2').length).to.equal(1); }); it('should change container on prop change', () => { @@ -165,82 +162,91 @@ describe('', () => { ); } - const wrapper = mount(); - assert.strictEqual(document.querySelector('#test3').parentElement.nodeName, 'SPAN', 'c'); - wrapper.setProps({ + const { setProps } = render(); + expect(document.querySelector('#test3').parentElement.nodeName).to.equal('SPAN'); + setProps({ containerElement: true, + disablePortal: true, }); - assert.strictEqual(document.querySelector('#test3').parentElement.nodeName, 'SPAN', 'a'); - wrapper.setProps({ + expect(document.querySelector('#test3').parentElement.nodeName).to.equal('SPAN'); + setProps({ + containerElement: true, disablePortal: false, }); - assert.strictEqual(document.querySelector('#test3').parentElement.nodeName, 'STRONG', 'c'); - wrapper.setProps({ + expect(document.querySelector('#test3').parentElement.nodeName).to.equal('STRONG'); + setProps({ containerElement: false, + disablePortal: false, }); - assert.strictEqual(document.querySelector('#test3').parentElement.nodeName, 'BODY', 'b'); + expect(document.querySelector('#test3').parentElement.nodeName).to.equal('BODY'); }); it('should call onRendered', () => { const ref = React.createRef(); const handleRendered = spy(); - mount( + render( { handleRendered(); - assert.strictEqual(ref.current !== null, true); + expect(ref.current !== null).to.equal(true); }} >
, ); - assert.strictEqual(handleRendered.callCount, 1); + expect(handleRendered.callCount).to.equal(1); }); - it('should call onRendered after child componentDidUpdate', () => { + it('should call ref after child effect', () => { + const callOrder = []; + const handleRef = node => { + if (node) { + callOrder.push('ref'); + } + }; + const updateFunction = () => { + callOrder.push('effect'); + }; + function Test(props) { - const { updateFunction, container, onRendered } = props; + const { container } = props; React.useEffect(() => { updateFunction(); - }, [container, updateFunction]); + }, [container]); return ( - +
); } - const callOrder = []; - const onRendered = () => { - callOrder.push('onRendered'); - }; - const updateFunction = () => { - callOrder.push('cDU'); - }; - - const container1 = document.createElement('div'); - const container2 = document.createElement('div'); + const { setProps } = render(); + + setProps({ container: null }); + setProps({ container: document.createElement('div') }); + setProps({ container: null }); + + expect(callOrder).to.deep.equal([ + 'effect', + 'ref', + 'effect', + 'ref', + 'effect', + 'ref', + 'effect', + 'ref', + ]); + }); - const wrapper = mount( - , + it('should render if children is undefined', () => { + render( +
+ + +
, ); - - wrapper.setProps({ container: null }); - wrapper.setProps({ container: container2 }); - wrapper.setProps({ container: null }); - - assert.deepEqual(callOrder, [ - 'cDU', - 'onRendered', - 'cDU', - 'onRendered', - 'cDU', - 'onRendered', - 'cDU', - 'onRendered', - ]); }); }); From 0267f63464a3f236170410c91a493332ab711590 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Thu, 5 Dec 2019 15:02:01 +0100 Subject: [PATCH 3/4] Sebastian review --- docs/pages/api/portal.md | 2 +- packages/material-ui/src/Portal/Portal.d.ts | 2 +- packages/material-ui/src/Portal/Portal.js | 14 ++++++-------- packages/material-ui/src/Portal/Portal.test.js | 9 --------- 4 files changed, 8 insertions(+), 19 deletions(-) diff --git a/docs/pages/api/portal.md b/docs/pages/api/portal.md index 8f47b6dbbca1c7..c4f4cb7c25995c 100644 --- a/docs/pages/api/portal.md +++ b/docs/pages/api/portal.md @@ -25,7 +25,7 @@ that exists outside the DOM hierarchy of the parent component. | Name | Type | Default | Description | |:-----|:-----|:--------|:------------| -| children | node | | The children to render into the `container`. | +| children * | node | | The children to render into the `container`. | | container | func
| React.Component
| Element | | A node, component instance, or function that returns either. The `container` will have the portal children appended to it. By default, it uses the body of the top-level document object, so it's simply `document.body` most of the time. | | disablePortal | bool | false | Disable the portal behavior. The children stay within it's parent DOM hierarchy. | | onRendered | func | | Callback fired once the children has been mounted into the `container`.
This prop will be deprecated and removed in v5, the ref can be used instead. | diff --git a/packages/material-ui/src/Portal/Portal.d.ts b/packages/material-ui/src/Portal/Portal.d.ts index 995037cba06db0..783e1a26c332f8 100644 --- a/packages/material-ui/src/Portal/Portal.d.ts +++ b/packages/material-ui/src/Portal/Portal.d.ts @@ -4,7 +4,7 @@ export interface PortalProps { /** * The children to render into the `container`. */ - children?: React.ReactNode; + children: React.ReactNode; /** * A node, component instance, or function that returns either. * The `container` will have the portal children appended to it. diff --git a/packages/material-ui/src/Portal/Portal.js b/packages/material-ui/src/Portal/Portal.js index 67b8a8a9a6af41..e9622e844f676b 100644 --- a/packages/material-ui/src/Portal/Portal.js +++ b/packages/material-ui/src/Portal/Portal.js @@ -20,7 +20,7 @@ const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect const Portal = React.forwardRef(function Portal(props, ref) { const { children, container, disablePortal = false, onRendered } = props; const [mountNode, setMountNode] = React.useState(null); - const handleRef = useForkRef(React.isValidElement(children) ? children.ref : null, ref); + const handleRef = useForkRef(children.ref, ref); useEnhancedEffect(() => { if (!disablePortal) { @@ -46,12 +46,10 @@ const Portal = React.forwardRef(function Portal(props, ref) { }, [onRendered, mountNode, disablePortal]); if (disablePortal) { - if (React.isValidElement(children)) { - return React.cloneElement(children, { - ref: handleRef, - }); - } - return children; + React.Children.only(children); + return React.cloneElement(children, { + ref: handleRef, + }); } return mountNode ? ReactDOM.createPortal(children, mountNode) : mountNode; @@ -65,7 +63,7 @@ Portal.propTypes = { /** * The children to render into the `container`. */ - children: PropTypes.node, + children: PropTypes.node.isRequired, /** * A node, component instance, or function that returns either. * The `container` will have the portal children appended to it. diff --git a/packages/material-ui/src/Portal/Portal.test.js b/packages/material-ui/src/Portal/Portal.test.js index 0825b1a6347cee..3cc62dc83d1239 100644 --- a/packages/material-ui/src/Portal/Portal.test.js +++ b/packages/material-ui/src/Portal/Portal.test.js @@ -240,13 +240,4 @@ describe('', () => { 'ref', ]); }); - - it('should render if children is undefined', () => { - render( -
- - -
, - ); - }); }); From 58b97cd33ed3ae5eae2959e2287b5171fa0f8f0c Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Thu, 5 Dec 2019 17:04:41 +0100 Subject: [PATCH 4/4] Process Kristoffer input --- docs/pages/api/portal.md | 2 +- packages/material-ui/src/Portal/Portal.d.ts | 2 +- packages/material-ui/src/Portal/Portal.js | 14 ++++++++------ packages/material-ui/src/Portal/Portal.test.js | 5 +++-- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/docs/pages/api/portal.md b/docs/pages/api/portal.md index c4f4cb7c25995c..8f47b6dbbca1c7 100644 --- a/docs/pages/api/portal.md +++ b/docs/pages/api/portal.md @@ -25,7 +25,7 @@ that exists outside the DOM hierarchy of the parent component. | Name | Type | Default | Description | |:-----|:-----|:--------|:------------| -| children * | node | | The children to render into the `container`. | +| children | node | | The children to render into the `container`. | | container | func
| React.Component
| Element | | A node, component instance, or function that returns either. The `container` will have the portal children appended to it. By default, it uses the body of the top-level document object, so it's simply `document.body` most of the time. | | disablePortal | bool | false | Disable the portal behavior. The children stay within it's parent DOM hierarchy. | | onRendered | func | | Callback fired once the children has been mounted into the `container`.
This prop will be deprecated and removed in v5, the ref can be used instead. | diff --git a/packages/material-ui/src/Portal/Portal.d.ts b/packages/material-ui/src/Portal/Portal.d.ts index 783e1a26c332f8..995037cba06db0 100644 --- a/packages/material-ui/src/Portal/Portal.d.ts +++ b/packages/material-ui/src/Portal/Portal.d.ts @@ -4,7 +4,7 @@ export interface PortalProps { /** * The children to render into the `container`. */ - children: React.ReactNode; + children?: React.ReactNode; /** * A node, component instance, or function that returns either. * The `container` will have the portal children appended to it. diff --git a/packages/material-ui/src/Portal/Portal.js b/packages/material-ui/src/Portal/Portal.js index e9622e844f676b..67b8a8a9a6af41 100644 --- a/packages/material-ui/src/Portal/Portal.js +++ b/packages/material-ui/src/Portal/Portal.js @@ -20,7 +20,7 @@ const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect const Portal = React.forwardRef(function Portal(props, ref) { const { children, container, disablePortal = false, onRendered } = props; const [mountNode, setMountNode] = React.useState(null); - const handleRef = useForkRef(children.ref, ref); + const handleRef = useForkRef(React.isValidElement(children) ? children.ref : null, ref); useEnhancedEffect(() => { if (!disablePortal) { @@ -46,10 +46,12 @@ const Portal = React.forwardRef(function Portal(props, ref) { }, [onRendered, mountNode, disablePortal]); if (disablePortal) { - React.Children.only(children); - return React.cloneElement(children, { - ref: handleRef, - }); + if (React.isValidElement(children)) { + return React.cloneElement(children, { + ref: handleRef, + }); + } + return children; } return mountNode ? ReactDOM.createPortal(children, mountNode) : mountNode; @@ -63,7 +65,7 @@ Portal.propTypes = { /** * The children to render into the `container`. */ - children: PropTypes.node.isRequired, + children: PropTypes.node, /** * A node, component instance, or function that returns either. * The `container` will have the portal children appended to it. diff --git a/packages/material-ui/src/Portal/Portal.test.js b/packages/material-ui/src/Portal/Portal.test.js index 3cc62dc83d1239..49d9bce0a55b22 100644 --- a/packages/material-ui/src/Portal/Portal.test.js +++ b/packages/material-ui/src/Portal/Portal.test.js @@ -128,10 +128,11 @@ describe('', () => { it('should render overlay into container (document)', () => { render( -
+
+
, ); - expect(document.querySelectorAll('#test2').length).to.equal(1); + expect(document.querySelectorAll('.test2').length).to.equal(2); }); it('should render overlay into container (DOMNode)', () => {