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

[Modal] Fix to defer setting of exited state to Transition component #15266

Merged
merged 7 commits into from
Apr 11, 2019
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
79 changes: 33 additions & 46 deletions docs/src/pages/utils/modal/SimpleModal.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
import PropTypes from 'prop-types';
import { withStyles } from '@material-ui/core/styles';
import { makeStyles } from '@material-ui/core/styles';
import Typography from '@material-ui/core/Typography';
import Modal from '@material-ui/core/Modal';
import Button from '@material-ui/core/Button';
Expand All @@ -20,7 +19,7 @@ function getModalStyle() {
};
}

const styles = theme => ({
const useStyles = makeStyles(theme => ({
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
paper: {
position: 'absolute',
width: 400,
Expand All @@ -29,54 +28,42 @@ const styles = theme => ({
padding: theme.spacing(4),
outline: 'none',
},
});
}));

class SimpleModal extends React.Component {
state = {
open: false,
};
function SimpleModal() {
const [open, setOpen] = React.useState(false);

handleOpen = () => {
this.setState({ open: true });
const handleOpen = () => {
setOpen(true);
};

handleClose = () => {
this.setState({ open: false });
const handleClose = () => {
setOpen(false);
};
const classes = useStyles();

render() {
const { classes } = this.props;

return (
<div>
<Typography gutterBottom>Click to get the full Modal experience!</Typography>
<Button onClick={this.handleOpen}>Open Modal</Button>
<Modal
aria-labelledby="simple-modal-title"
aria-describedby="simple-modal-description"
open={this.state.open}
onClose={this.handleClose}
>
<div style={getModalStyle()} className={classes.paper}>
<Typography variant="h6" id="modal-title">
Text in a modal
</Typography>
<Typography variant="subtitle1" id="simple-modal-description">
Duis mollis, est non commodo luctus, nisi erat porttitor ligula.
</Typography>
<SimpleModalWrapped />
</div>
</Modal>
</div>
);
}
return (
<div>
<Typography gutterBottom>Click to get the full Modal experience!</Typography>
<Button onClick={handleOpen}>Open Modal</Button>
<Modal
aria-labelledby="simple-modal-title"
aria-describedby="simple-modal-description"
open={open}
onClose={handleClose}
>
<div style={getModalStyle()} className={classes.paper}>
<Typography variant="h6" id="modal-title">
Text in a modal
</Typography>
<Typography variant="subtitle1" id="simple-modal-description">
Duis mollis, est non commodo luctus, nisi erat porttitor ligula.
</Typography>
<SimpleModal />
</div>
</Modal>
</div>
);
}

SimpleModal.propTypes = {
classes: PropTypes.object.isRequired,
};

// We need an intermediary variable for handling the recursive nesting.
const SimpleModalWrapped = withStyles(styles)(SimpleModal);

export default SimpleModalWrapped;
export default SimpleModal;
6 changes: 4 additions & 2 deletions packages/material-ui-lab/src/SpeedDial/SpeedDial.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,9 @@ describe('<SpeedDial />', () => {
resetDialToOpen(dialDirection);

getDialButton().simulate('keydown', { keyCode: keycodes[firstKey] });
assert.isTrue(
assert.strictEqual(
isActionFocused(firstFocusedAction),
true,
`focused action initial ${firstKey} should be ${firstFocusedAction}`,
);

Expand All @@ -363,8 +364,9 @@ describe('<SpeedDial />', () => {
getActionButton(previousFocusedAction).simulate('keydown', {
keyCode: keycodes[arrowKey],
});
assert.isTrue(
assert.strictEqual(
isActionFocused(expectedFocusedAction),
true,
`focused action after ${combinationUntilNot.join(
',',
)} should be ${expectedFocusedAction}`,
Expand Down
24 changes: 6 additions & 18 deletions packages/material-ui/src/Modal/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,6 @@ class Modal extends React.Component {
}
}

static getDerivedStateFromProps(nextProps) {
if (nextProps.open) {
return {
exited: false,
};
}

if (!getHasTransition(nextProps)) {
// Otherwise let handleExited take care of marking exited.
return {
exited: true,
};
}

return null;
}

handleOpen = () => {
const container = getContainer(this.props.container) || this.getDoc().body;

Expand Down Expand Up @@ -134,6 +117,10 @@ class Modal extends React.Component {
}
};

handleEnter = () => {
this.setState({ exited: false });
};

handleExited = () => {
if (this.props.closeAfterTransition) {
this.props.manager.remove(this);
Expand Down Expand Up @@ -232,6 +219,7 @@ class Modal extends React.Component {

// It's a Transition like component
if (hasTransition) {
childProps.onEnter = createChainedFunction(this.handleEnter, children.props.onEnter);
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
childProps.onExited = createChainedFunction(this.handleExited, children.props.onExited);
}

Expand Down Expand Up @@ -262,7 +250,7 @@ class Modal extends React.Component {
onKeyDown={this.handleKeyDown}
role="presentation"
className={clsx(classes.root, className, {
[classes.hidden]: exited,
[classes.hidden]: !open && exited,
})}
{...other}
>
Expand Down
37 changes: 37 additions & 0 deletions packages/material-ui/src/Modal/Modal.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,43 @@ describe('<Modal />', () => {
const modalNode = modalRef.current;
assert.strictEqual(modalNode.getAttribute('aria-hidden'), 'true');
});

// Test case for https://github.com/mui-org/material-ui/issues/15180
it('should remove the transition children in the DOM when closed whilst transition status is entering', () => {
const children = <p>Hello World</p>;

class OpenClose extends React.Component {
state = {
open: false,
};

handleClick = () => {
this.setState({ open: true }, () => {
this.setState({ open: false });
});
};

render() {
return (
<div>
<button type="button" onClick={this.handleClick}>
Toggle Tooltip
</button>
<Modal open={this.state.open}>
<Fade in={this.state.open}>
<span>{children}</span>
</Fade>
</Modal>
</div>
);
}
}

const wrapper = mount(<OpenClose />);
assert.strictEqual(wrapper.contains(children), false);
wrapper.find('button').simulate('click');
assert.strictEqual(wrapper.contains(children), false);
});
});

describe('focus', () => {
Expand Down