review

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

@@ -115,6 +115,10 @@ const Modal = React.forwardRef(function Modal(props, ref) {
   const handlePortalRef = useEventCallback(node => {
     mountNodeRef.current = node;
 
+    if (!node) {
+      return;
+    }
+
     if (onRendered) {
       onRendered();
     }
@@ -324,8 +328,7 @@ Modal.propTypes = {
   /**
    * @ignore
    *
-   * A modal manager used to track and manage the state of open
-   * Modals. This enables customizing how modals interact within a container.
+   * A modal manager used to track and manage the state of open Modals.
    */
   manager: PropTypes.object,
   /**
@@ -348,6 +351,8 @@ Modal.propTypes = {
   /**
    * Callback fired once the children has been mounted into the `container`.
    * It signals that the `open={true}` property took effect.
+   *
+   * TODO v5: to remove, the ref can be used instead.
    */
   onRendered: PropTypes.func,
   /**

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

@@ -19,8 +19,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 childRef = React.useRef(null);
-  const handleRef = useForkRef(children.ref, childRef);
+  const handleRef = useForkRef(children.ref, ref);
 
   useEnhancedEffect(() => {
     if (!disablePortal) {
@@ -29,20 +28,21 @@ const Portal = React.forwardRef(function Portal(props, ref) {
   }, [container, disablePortal]);
 
   useEnhancedEffect(() => {
-    if (mountNode || disablePortal) {
-      setRef(ref, mountNode || childRef.current);
+    if (mountNode && !disablePortal) {
+      setRef(ref, mountNode);
+      return () => {
+        setRef(ref, null);
+      };
     }
 
-    return () => {
-      setRef(null);
-    };
-  }, [ref, mountNode]);
+    return undefined;
+  }, [ref, mountNode, disablePortal]);
 
   useEnhancedEffect(() => {
     if (onRendered && (mountNode || disablePortal)) {
       onRendered();
     }
-  }, [mountNode, onRendered, disablePortal]);
+  }, [onRendered, mountNode, disablePortal]);
 
   if (disablePortal) {
     React.Children.only(children);
@@ -73,8 +73,10 @@ Portal.propTypes = {
   disablePortal: PropTypes.bool,
   /**
    * Callback fired once the children has been mounted into the `container`.
+   *
+   * TODO v5: to remove, the ref can be used instead.
    */
-  onRendered: PropTypes.func, // TODO v5: to remove
+  onRendered: PropTypes.func,
 };
 
 if (process.env.NODE_ENV !== 'production') {

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

@@ -48,20 +48,39 @@ describe('<Portal />', () => {
   });
 
   it('should have access to the mountNode', () => {
+    let wrapper;
+    let mountNode;
     const refSpy1 = spy();
-    mount(
+    wrapper = mount(
       <Portal ref={refSpy1}>
         <h1>Foo</h1>
       </Portal>,
     );
-    assert.deepEqual(refSpy1.args, [[document.body]]);
+    wrapper.unmount();
+    assert.deepEqual(refSpy1.args, [[document.body], [null]]);
+
     const refSpy2 = spy();
-    mount(
+    wrapper = mount(
       <Portal disablePortal ref={refSpy2}>
         <h1 className="woofPortal">Foo</h1>
       </Portal>,
     );
-    assert.deepEqual(refSpy2.args, [[document.querySelector('.woofPortal')]]);
+    mountNode = document.querySelector('.woofPortal');
+    wrapper.unmount();
+    assert.deepEqual(refSpy2.args, [[mountNode], [null]]);
+
+    const refSpy3 = spy();
+    wrapper = mount(
+      <Portal disablePortal ref={refSpy3}>
+        <h1 className="woofPortal">Foo</h1>
+      </Portal>,
+    );
+    mountNode = document.querySelector('.woofPortal');
+    wrapper.setProps({
+      disablePortal: false,
+    });
+    wrapper.unmount();
+    assert.deepEqual(refSpy3.args, [[mountNode], [null], [document.body], [null]]);
   });
 
   it('should render in a different node', () => {

pages/api/modal.md

@@ -44,7 +44,7 @@ This component shares many concepts with [react-overlays](https://react-bootstra
 | <span class="prop-name">onBackdropClick</span> | <span class="prop-type">func</span> |  | Callback fired when the backdrop is clicked. |
 | <span class="prop-name">onClose</span> | <span class="prop-type">func</span> |  | Callback fired when the component requests to be closed. The `reason` parameter can optionally be used to control the response to `onClose`.<br><br>**Signature:**<br>`function(event: object, reason: string) => void`<br>*event:* The event source of the callback<br>*reason:* Can be:`"escapeKeyDown"`, `"backdropClick"` |
 | <span class="prop-name">onEscapeKeyDown</span> | <span class="prop-type">func</span> |  | Callback fired when the escape key is pressed, `disableEscapeKeyDown` is false and the modal is in focus. |
-| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`. It signals that the `open={true}` property took effect. |
+| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`. It signals that the `open={true}` property took effect.<br>TODO v5: to remove, the ref can be used instead. |
 | <span class="prop-name required">open&nbsp;*</span> | <span class="prop-type">bool</span> |  | If `true`, the modal is open. |
 
 The `ref` is forwarded to the root element.

pages/api/portal.md

@@ -22,7 +22,7 @@ that exists outside the DOM hierarchy of the parent component.
 | <span class="prop-name required">children&nbsp;*</span> | <span class="prop-type">node</span> |  | The children to render into the `container`. |
 | <span class="prop-name">container</span> | <span class="prop-type">union:&nbsp;object&nbsp;&#124;<br>&nbsp;func<br></span> |  | 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. |
 | <span class="prop-name">disablePortal</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | Disable the portal behavior. The children stay within it's parent DOM hierarchy. |
-| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`. |
+| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> |  | Callback fired once the children has been mounted into the `container`.<br>TODO v5: to remove, the ref can be used instead. |
 
 The component cannot hold a ref.