From 343ffd4c74764d535b5526a5f86b4fca2ce4bafa Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Tue, 22 Dec 2020 14:05:37 +0100 Subject: [PATCH] [Modal][Portal] Deprecate onRendered (#24082) --- docs/pages/api-docs/modal.md | 2 +- docs/pages/api-docs/portal.md | 2 +- packages/material-ui/src/Modal/Modal.d.ts | 10 ++++ packages/material-ui/src/Modal/Modal.js | 5 +- packages/material-ui/src/Modal/Modal.test.js | 17 ++++++- packages/material-ui/src/Portal/Portal.d.ts | 3 +- packages/material-ui/src/Portal/Portal.js | 6 ++- .../material-ui/src/Portal/Portal.test.js | 46 +++++++++++++------ 8 files changed, 67 insertions(+), 24 deletions(-) diff --git a/docs/pages/api-docs/modal.md b/docs/pages/api-docs/modal.md index aef58d703ac875..72752da60972c1 100644 --- a/docs/pages/api-docs/modal.md +++ b/docs/pages/api-docs/modal.md @@ -53,7 +53,7 @@ This component shares many concepts with [react-overlays](https://react-bootstra | onBackdropClick | func | | Callback fired when the backdrop is clicked. | | onClose | func | | Callback fired when the component requests to be closed. The `reason` parameter can optionally be used to control the response to `onClose`.

**Signature:**
`function(event: object, reason: string) => void`
*event:* The event source of the callback.
*reason:* Can be: `"escapeKeyDown"`, `"backdropClick"`. | | onEscapeKeyDown | func | | Callback fired when the escape key is pressed, `disableEscapeKeyDown` is false and the modal is in focus. | -| onRendered | func | | Callback fired once the children has been mounted into the `container`. It signals that the `open={true}` prop took effect.
This prop will be deprecated and removed in v5, the ref can be used instead. | +| ~~onRendered~~ | func | | *Deprecated*. Use the ref instead.

Callback fired once the children has been mounted into the `container`. It signals that the `open={true}` prop took effect.
This prop will be removed in v5, the ref can be used instead. | | open* | bool | | If `true`, the modal is open. | The `ref` is forwarded to the root element. diff --git a/docs/pages/api-docs/portal.md b/docs/pages/api-docs/portal.md index 6e6892edc27ffe..c83882d6d6e140 100644 --- a/docs/pages/api-docs/portal.md +++ b/docs/pages/api-docs/portal.md @@ -30,7 +30,7 @@ that exists outside the DOM hierarchy of the parent component. | children | node | | The children to render into the `container`. | | container | HTML element
| React.Component
| func | | A HTML element, 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. | +| ~~onRendered~~ | func | | *Deprecated*. Use the ref instead.

Callback fired once the children has been mounted into the `container`.
This prop will be removed in v5, the ref can be used instead. | The component cannot hold a ref. diff --git a/packages/material-ui/src/Modal/Modal.d.ts b/packages/material-ui/src/Modal/Modal.d.ts index 8d22a51e86e4be..276ad1b1649be2 100644 --- a/packages/material-ui/src/Modal/Modal.d.ts +++ b/packages/material-ui/src/Modal/Modal.d.ts @@ -31,7 +31,17 @@ export interface ModalProps bivarianceHack(event: {}, reason: 'backdropClick' | 'escapeKeyDown'): void; }['bivarianceHack']; onEscapeKeyDown?: React.ReactEventHandler<{}>; + /** + * Callback fired once the children has been mounted into the `container`. + * It signals that the `open={true}` prop took effect. + * + * This prop will be removed in v5, the ref can be used instead. + * @deprecated Use the ref instead. + */ onRendered?: PortalProps['onRendered']; + /** + * If `true`, the modal is open. + */ open: boolean; } diff --git a/packages/material-ui/src/Modal/Modal.js b/packages/material-ui/src/Modal/Modal.js index a5e4a69be4b80c..3051d2bfb6ef8c 100644 --- a/packages/material-ui/src/Modal/Modal.js +++ b/packages/material-ui/src/Modal/Modal.js @@ -4,6 +4,7 @@ import PropTypes from 'prop-types'; import { getThemeProps, useTheme } from '@material-ui/styles'; import { elementAcceptingRef, HTMLElementType } from '@material-ui/utils'; import ownerDocument from '../utils/ownerDocument'; +import deprecatedPropType from '../utils/deprecatedPropType'; import Portal from '../Portal'; import createChainedFunction from '../utils/createChainedFunction'; import useForkRef from '../utils/useForkRef'; @@ -359,9 +360,9 @@ Modal.propTypes = { * Callback fired once the children has been mounted into the `container`. * It signals that the `open={true}` prop took effect. * - * This prop will be deprecated and removed in v5, the ref can be used instead. + * This prop will be removed in v5, the ref can be used instead. */ - onRendered: PropTypes.func, + onRendered: deprecatedPropType(PropTypes.func, 'Use the ref instead.'), /** * If `true`, the modal is open. */ diff --git a/packages/material-ui/src/Modal/Modal.test.js b/packages/material-ui/src/Modal/Modal.test.js index 0a9efe8afd9558..1d18c3bd3e4dcb 100644 --- a/packages/material-ui/src/Modal/Modal.test.js +++ b/packages/material-ui/src/Modal/Modal.test.js @@ -527,8 +527,17 @@ describe('', () => { }); }); - describe('prop: onRendered', () => { - it('should fire', () => { + describe('v5 deprecations', () => { + beforeEach(() => { + PropTypes.resetWarningCache(); + consoleErrorMock.spy(); + }); + + afterEach(() => { + consoleErrorMock.reset(); + }); + + it('should call onRendered', () => { const handleRendered = spy(); render( @@ -538,6 +547,10 @@ describe('', () => { ); expect(handleRendered).to.have.property('callCount', 1); + expect(console.error.callCount).to.equal(1); + expect(console.error.firstCall.args[0]).to.contain( + 'The prop `onRendered` of `ForwardRef(Modal)` is deprecated. Use the ref instead', + ); }); }); diff --git a/packages/material-ui/src/Portal/Portal.d.ts b/packages/material-ui/src/Portal/Portal.d.ts index 0faa9b73f9dad9..e0254b75e85d3d 100644 --- a/packages/material-ui/src/Portal/Portal.d.ts +++ b/packages/material-ui/src/Portal/Portal.d.ts @@ -21,7 +21,8 @@ export interface PortalProps { /** * 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. + * This prop will be removed in v5, the ref can be used instead. + * @deprecated Use the ref instead. */ onRendered?: () => void; } diff --git a/packages/material-ui/src/Portal/Portal.js b/packages/material-ui/src/Portal/Portal.js index af9259520c82d8..ec4324277d6f0f 100644 --- a/packages/material-ui/src/Portal/Portal.js +++ b/packages/material-ui/src/Portal/Portal.js @@ -2,6 +2,7 @@ import * as React from 'react'; import * as ReactDOM from 'react-dom'; import PropTypes from 'prop-types'; import { exactProp, HTMLElementType } from '@material-ui/utils'; +import deprecatedPropType from '../utils/deprecatedPropType'; import setRef from '../utils/setRef'; import useForkRef from '../utils/useForkRef'; @@ -86,9 +87,10 @@ Portal.propTypes = { /** * 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. + * This prop will be removed in v5, the ref can be used instead. + * @deprecated Use the ref instead. */ - onRendered: PropTypes.func, + onRendered: deprecatedPropType(PropTypes.func, 'Use the ref instead.'), }; if (process.env.NODE_ENV !== 'production') { diff --git a/packages/material-ui/src/Portal/Portal.test.js b/packages/material-ui/src/Portal/Portal.test.js index 4e47d3fc61b0e5..623ee83c2edfa6 100644 --- a/packages/material-ui/src/Portal/Portal.test.js +++ b/packages/material-ui/src/Portal/Portal.test.js @@ -1,6 +1,7 @@ import * as React from 'react'; import { expect } from 'chai'; import { spy } from 'sinon'; +import PropTypes from 'prop-types'; import createServerRender from 'test/utils/createServerRender'; import consoleErrorMock from 'test/utils/consoleErrorMock'; import { createClientRender } from 'test/utils/createClientRender'; @@ -177,21 +178,36 @@ describe('', () => { expect(document.querySelector('#test3').parentElement.nodeName).to.equal('BODY'); }); - it('should call onRendered', () => { - const ref = React.createRef(); - const handleRendered = spy(); - render( - { - handleRendered(); - expect(ref.current !== null).to.equal(true); - }} - > -
- , - ); - expect(handleRendered.callCount).to.equal(1); + describe('v5 deprecations', () => { + beforeEach(() => { + PropTypes.resetWarningCache(); + consoleErrorMock.spy(); + }); + + afterEach(() => { + consoleErrorMock.reset(); + }); + + it('should call onRendered', () => { + const ref = React.createRef(); + const handleRendered = spy(); + render( + { + handleRendered(); + expect(ref.current !== null).to.equal(true); + }} + > +
+ , + ); + expect(handleRendered.callCount).to.equal(1); + expect(console.error.callCount).to.equal(1); + expect(console.error.firstCall.args[0]).to.contain( + 'The prop `onRendered` of `ForwardRef(Portal)` is deprecated. Use the ref instead', + ); + }); }); it('should call ref after child effect', () => {