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

[Dialog][Modal] Remove disableBackdropClick #23607

Merged
merged 6 commits into from
Nov 20, 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/dialog.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ The `MuiDialog` name can be used for providing [default props](/customization/gl
| <span class="prop-name">aria-labelledby</span> | <span class="prop-type">string</span> | | The id(s) of the element(s) that label the dialog. |
| <span class="prop-name">children</span> | <span class="prop-type">node</span> | | Dialog children, usually the included sub-components. |
| <span class="prop-name">classes</span> | <span class="prop-type">object</span> | | Override or extend the styles applied to the component. See [CSS API](#css) below for more details. |
| <span class="prop-name">disableBackdropClick</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, clicking the backdrop will not fire the `onClose` callback. |
| <span class="prop-name">disableEscapeKeyDown</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, hitting escape will not fire the `onClose` callback. |
| <span class="prop-name">fullScreen</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the dialog is full-screen. |
| <span class="prop-name">fullWidth</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the dialog stretches to `maxWidth`.<br>Notice that the dialog width grow is limited by the default margin. |
Expand Down
1 change: 0 additions & 1 deletion docs/pages/api-docs/modal.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ This component shares many concepts with [react-overlays](https://react-bootstra
| <span class="prop-name">closeAfterTransition</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | When set to true the Modal waits until a nested Transition is completed before closing. |
| <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">disableAutoFocus</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the modal will not automatically shift focus to itself when it opens, and replace it to the last focused element when it closes. This also works correctly with any modal children that have the `disableAutoFocus` prop.<br>Generally this should never be set to `true` as it makes the modal less accessible to assistive technologies, like screen readers. |
| <span class="prop-name">disableBackdropClick</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, clicking the backdrop will not fire `onClose`. |
| <span class="prop-name">disableEnforceFocus</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the modal will not prevent focus from leaving the modal while open.<br>Generally this should never be set to `true` as it makes the modal less accessible to assistive technologies, like screen readers. |
| <span class="prop-name">disableEscapeKeyDown</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, hitting escape will not fire `onClose`. |
| <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. |
Expand Down
2 changes: 0 additions & 2 deletions docs/src/pages/components/dialogs/ConfirmationDialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ function ConfirmationDialogRaw(props) {

return (
<Dialog
disableBackdropClick
disableEscapeKeyDown
Comment on lines -64 to -65
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't have any effect on next either because we already have a fully custom onClose in ConfirmationDialogRaw

maxWidth="xs"
TransitionProps={{ onEntering: handleEntering }}
aria-labelledby="confirmation-dialog-title"
Expand Down
2 changes: 0 additions & 2 deletions docs/src/pages/components/dialogs/ConfirmationDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ function ConfirmationDialogRaw(props: ConfirmationDialogRawProps) {

return (
<Dialog
disableBackdropClick
disableEscapeKeyDown
maxWidth="xs"
TransitionProps={{ onEntering: handleEntering }}
aria-labelledby="confirmation-dialog-title"
Expand Down
13 changes: 5 additions & 8 deletions docs/src/pages/components/selects/DialogSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,16 @@ export default function DialogSelect() {
setOpen(true);
};

const handleClose = () => {
setOpen(false);
const handleClose = (event, reason) => {
if (reason !== 'backdropClick') {
setOpen(false);
}
};

return (
<div>
<Button onClick={handleClickOpen}>Open select dialog</Button>
<Dialog
disableBackdropClick
disableEscapeKeyDown
open={open}
onClose={handleClose}
>
<Dialog disableEscapeKeyDown open={open} onClose={handleClose}>
<DialogTitle>Fill the form</DialogTitle>
<DialogContent>
<form className={classes.container}>
Expand Down
16 changes: 8 additions & 8 deletions docs/src/pages/components/selects/DialogSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,19 @@ export default function DialogSelect() {
setOpen(true);
};

const handleClose = () => {
setOpen(false);
const handleClose = (
event: React.SyntheticEvent<unknown>,
reason?: string,
) => {
if (reason !== 'backdropClick') {
setOpen(false);
}
};

return (
<div>
<Button onClick={handleClickOpen}>Open select dialog</Button>
<Dialog
disableBackdropClick
disableEscapeKeyDown
open={open}
onClose={handleClose}
>
<Dialog disableEscapeKeyDown open={open} onClose={handleClose}>
<DialogTitle>Fill the form</DialogTitle>
<DialogContent>
<form className={classes.container}>
Expand Down
44 changes: 44 additions & 0 deletions docs/src/pages/guides/migration-v4/migration-v4.md
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,21 @@ const classes = makeStyles(theme => ({
/>
```

- Remove the `disableBackdropClick` prop because redundant.
Ignore close events from `onClose` when `reason === 'backdropClick'` instead.

```diff
<Dialog
- disableBackdropClick
- onClose={handleClose}
+ onClose={(event, reason) => {
+ if (reason !== 'backdropClick') {
+ onClose(event, reason);
+ }
+ }}
/>
```

- [withMobileDialog] Remove this higher-order component. The hook API allows a simpler and more flexible solution:

```diff
Expand Down Expand Up @@ -585,6 +600,35 @@ const classes = makeStyles(theme => ({

### Modal

- Remove the `disableBackdropClick` prop because redundant.
Ignore close events from `onClose` when `reason === 'backdropClick'` instead.

```diff
<Modal
- disableBackdropClick
- onClose={handleClose}
+ onClose={(event, reason) => {
+ if (reason !== 'backdropClick') {
+ onClose(event, reason);
+ }
+ }}
/>
```

- Remove the `onEscapeKeyDown` prop because redundant.
Use `onClose` with `reason === "escapeKeyDown"` instead.

```diff
<Modal
- onEscapeKeyDown={handleEscapeKeyDown}
+ onClose={(event, reason) => {
+ if (reason === 'escapeKeyDown') {
+ handleEscapeKeyDown(event);
+ }
+ }}
/>
```

- 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.

Expand Down
5 changes: 0 additions & 5 deletions packages/material-ui/src/Dialog/Dialog.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ export interface DialogProps
/** Styles applied to the `Paper` component if `fullScreen={true}`. */
paperFullScreen?: string;
};
/**
* If `true`, clicking the backdrop will not fire the `onClose` callback.
* @default false
*/
disableBackdropClick?: boolean;
/**
* If `true`, hitting escape will not fire the `onClose` callback.
* @default false
Expand Down
9 changes: 1 addition & 8 deletions packages/material-ui/src/Dialog/Dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ const Dialog = React.forwardRef(function Dialog(props, ref) {
children,
classes,
className,
disableBackdropClick = false,
disableEscapeKeyDown = false,
fullScreen = false,
fullWidth = false,
Expand Down Expand Up @@ -182,7 +181,7 @@ const Dialog = React.forwardRef(function Dialog(props, ref) {
onBackdropClick(event);
}

if (!disableBackdropClick && onClose) {
if (onClose) {
onClose(event, 'backdropClick');
}
};
Expand All @@ -196,7 +195,6 @@ const Dialog = React.forwardRef(function Dialog(props, ref) {
...BackdropProps,
}}
closeAfterTransition
disableBackdropClick={disableBackdropClick}
disableEscapeKeyDown={disableEscapeKeyDown}
onClose={onClose}
open={open}
Expand Down Expand Up @@ -272,11 +270,6 @@ Dialog.propTypes = {
* @ignore
*/
className: PropTypes.string,
/**
* If `true`, clicking the backdrop will not fire the `onClose` callback.
* @default false
*/
disableBackdropClick: PropTypes.bool,
/**
* If `true`, hitting escape will not fire the `onClose` callback.
* @default false
Expand Down
15 changes: 12 additions & 3 deletions packages/material-ui/src/Dialog/Dialog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,26 @@ describe('<Dialog />', () => {
});

it('can ignore backdrop click and Esc keydown', () => {
function DialogWithBackdropClickDisabled(props) {
const { onClose, ...other } = props;
function handleClose(event, reason) {
if (reason !== 'backdropClick') {
onClose(event, reason);
}
}

return <Dialog onClose={handleClose} {...other} />;
}
const onClose = spy();
const { getByRole } = render(
<Dialog
<DialogWithBackdropClickDisabled
open
disableBackdropClick
disableEscapeKeyDown
onClose={onClose}
transitionDuration={0}
>
foo
</Dialog>,
</DialogWithBackdropClickDisabled>,
);
const dialog = getByRole('dialog');
expect(dialog).not.to.equal(null);
Expand Down
5 changes: 0 additions & 5 deletions packages/material-ui/src/Modal/Modal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ export interface ModalProps
* @default false
*/
disableAutoFocus?: boolean;
/**
* If `true`, clicking the backdrop will not fire `onClose`.
* @default false
*/
disableBackdropClick?: boolean;
/**
* If `true`, the modal will not prevent focus from leaving the modal while open.
*
Expand Down
8 changes: 1 addition & 7 deletions packages/material-ui/src/Modal/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
closeAfterTransition = false,
container,
disableAutoFocus = false,
disableBackdropClick = false,
disableEnforceFocus = false,
disableEscapeKeyDown = false,
disablePortal = false,
Expand Down Expand Up @@ -172,7 +171,7 @@ const Modal = React.forwardRef(function Modal(inProps, ref) {
onBackdropClick(event);
}

if (!disableBackdropClick && onClose) {
if (onClose) {
onClose(event, 'backdropClick');
}
};
Expand Down Expand Up @@ -296,11 +295,6 @@ Modal.propTypes = {
* @default false
*/
disableAutoFocus: PropTypes.bool,
/**
* If `true`, clicking the backdrop will not fire `onClose`.
* @default false
*/
disableBackdropClick: PropTypes.bool,
/**
* If `true`, the modal will not prevent focus from leaving the modal while open.
*
Expand Down
19 changes: 16 additions & 3 deletions packages/material-ui/src/Modal/Modal.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,16 +161,29 @@ describe('<Modal />', () => {
});

it('should let the user disable backdrop click triggering onClose', () => {
function ModalWithDisabledBackdropClick(props) {
const { onClose, ...other } = props;
function handleClose(event, reason) {
if (reason !== 'backdropClick') {
onClose(event, reason);
}
}

return (
<Modal onClose={handleClose} {...other}>
<div />
</Modal>
);
}
const onClose = spy();
const { getByTestId } = render(
<Modal
<ModalWithDisabledBackdropClick
onClose={onClose}
open
disableBackdropClick
BackdropProps={{ 'data-testid': 'backdrop' }}
>
<div />
</Modal>,
</ModalWithDisabledBackdropClick>,
);

getByTestId('backdrop').click();
Expand Down