Skip to content

Commit

Permalink
[core] Remove onRendered from Modal and Portal (#22464)
Browse files Browse the repository at this point in the history
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
  • Loading branch information
eps1lon and oliviertassinari authored Sep 4, 2020
1 parent d16b3c1 commit f5a5eca
Show file tree
Hide file tree
Showing 9 changed files with 12 additions and 73 deletions.
1 change: 0 additions & 1 deletion docs/pages/api-docs/modal.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ 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}` prop took effect.<br>This prop will be deprecated and removed in v5, the ref can be used instead. |
| <span class="prop-name required">open<abbr title="required">*</abbr></span> | <span class="prop-type">bool</span> | | If `true`, the modal is open. |

The `ref` is forwarded to the root element.
Expand Down
1 change: 0 additions & 1 deletion docs/pages/api-docs/portal.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ that exists outside the DOM hierarchy of the parent component.
| <span class="prop-name">children</span> | <span class="prop-type">node</span> | | The children to render into the `container`. |
| <span class="prop-name">container</span> | <span class="prop-type">HTML element<br>&#124;&nbsp;func</span> | | A HTML element or function that returns one. The `container` will have the portal children appended to it.<br>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> | The `children` will be inside the DOM hierarchy of the parent component. |
| <span class="prop-name">onRendered</span> | <span class="prop-type">func</span> | | Callback fired once the children has been mounted into the `container`.<br>This prop will be deprecated and removed in v5, the ref can be used instead. |

The component cannot hold a ref.

Expand Down
10 changes: 10 additions & 0 deletions docs/src/pages/guides/migration-v4/migration-v4.md
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,11 @@ const theme = createMuitheme({
>
```

### Modal

- Remove `onRendered` prop.
Depending on your use case either use a [callback ref](https://reactjs.org/docs/refs-and-the-dom.html#callback-refs) on the child element or an effect hook in the child component.

### Pagination

- Rename `round` to `circular` for consistency. The possible values should be adjectives, not nouns:
Expand Down Expand Up @@ -404,6 +409,11 @@ const theme = createMuitheme({
/>
```

### Portal

- Remove `onRendered` prop.
Depending on your use case either use a [callback ref](https://reactjs.org/docs/refs-and-the-dom.html#callback-refs) on the child element or an effect hook in the child component.

### Rating

- Rename `visuallyhidden` to `visuallyHidden` for consistency:
Expand Down
7 changes: 0 additions & 7 deletions packages/material-ui/src/Modal/Modal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,6 @@ export interface ModalProps
* `disableEscapeKeyDown` is false and the modal is in focus.
*/
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 deprecated and removed in v5, the ref can be used instead.
*/
onRendered?: PortalProps['onRendered'];
/**
* If `true`, the modal is open.
*/
Expand Down
12 changes: 0 additions & 12 deletions packages/material-ui/src/Modal/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
onBackdropClick,
onClose,
onEscapeKeyDown,
onRendered,
open,
...other
} = props;
Expand Down Expand Up @@ -123,10 +122,6 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
return;
}

if (onRendered) {
onRendered();
}

if (open && isTopModal()) {
handleMounted();
} else {
Expand Down Expand Up @@ -354,13 +349,6 @@ Modal.propTypes = {
* `disableEscapeKeyDown` is false and the modal is in focus.
*/
onEscapeKeyDown: PropTypes.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: PropTypes.func,
/**
* If `true`, the modal is open.
*/
Expand Down
14 changes: 0 additions & 14 deletions packages/material-ui/src/Modal/Modal.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -531,20 +531,6 @@ describe('<Modal />', () => {
});
});

describe('prop: onRendered', () => {
it('should fire', () => {
const handleRendered = spy();

render(
<Modal open onRendered={handleRendered}>
<div />
</Modal>,
);

expect(handleRendered).to.have.property('callCount', 1);
});
});

describe('two modal at the same time', () => {
/**
* @type {ReturnType<typeof useFakeTimers>}
Expand Down
6 changes: 0 additions & 6 deletions packages/material-ui/src/Portal/Portal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,6 @@ export interface PortalProps {
* The `children` will be inside the DOM hierarchy of the parent component.
*/
disablePortal?: boolean;
/**
* 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?: () => void;
}

/**
Expand Down
14 changes: 1 addition & 13 deletions packages/material-ui/src/Portal/Portal.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function getContainer(container) {
* that exists outside the DOM hierarchy of the parent component.
*/
const Portal = React.forwardRef(function Portal(props, ref) {
const { children, container, disablePortal = false, onRendered } = props;
const { children, container, disablePortal = false } = props;
const [mountNode, setMountNode] = React.useState(null);
const handleRef = useForkRef(React.isValidElement(children) ? children.ref : null, ref);

Expand All @@ -36,12 +36,6 @@ const Portal = React.forwardRef(function Portal(props, ref) {
return undefined;
}, [ref, mountNode, disablePortal]);

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

if (disablePortal) {
if (React.isValidElement(children)) {
return React.cloneElement(children, {
Expand Down Expand Up @@ -78,12 +72,6 @@ Portal.propTypes = {
* The `children` will be inside the DOM hierarchy of the parent component.
*/
disablePortal: PropTypes.bool,
/**
* 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: PropTypes.func,
};

if (process.env.NODE_ENV !== 'production') {
Expand Down
20 changes: 1 addition & 19 deletions packages/material-ui/src/Portal/Portal.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,10 @@ describe('<Portal />', () => {
);
}).toErrorDev(
// Known issue due to using SSR APIs in a browser environment.
// We use 3x useLayoutEffect in the component.
// We use 2x useLayoutEffect in the component.
[
'Warning: useLayoutEffect does nothing on the server',
'Warning: useLayoutEffect does nothing on the server',
'Warning: useLayoutEffect does nothing on the server',
],
);
expect(markup2.text()).to.equal('');
Expand Down Expand Up @@ -178,23 +177,6 @@ describe('<Portal />', () => {
expect(document.querySelector('#test3').parentElement.nodeName).to.equal('BODY');
});

it('should call onRendered', () => {
const ref = React.createRef();
const handleRendered = spy();
render(
<Portal
ref={ref}
onRendered={() => {
handleRendered();
expect(ref.current !== null).to.equal(true);
}}
>
<div />
</Portal>,
);
expect(handleRendered.callCount).to.equal(1);
});

it('should call ref after child effect', () => {
const callOrder = [];
const handleRef = (node) => {
Expand Down

0 comments on commit f5a5eca

Please sign in to comment.