Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Remove onRendered from Modal and Portal #22464

Merged
merged 3 commits into from
Sep 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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