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

Allows for the modal to be appended to another element rather than the document body. #183

Merged
merged 12 commits into from
Dec 6, 2016
37 changes: 33 additions & 4 deletions lib/components/Modal.js
Original file line number Diff line number Diff line change
@@ -10,6 +10,18 @@ var Assign = require('lodash.assign');
var SafeHTMLElement = ExecutionEnvironment.canUseDOM ? window.HTMLElement : {};
var AppElement = ExecutionEnvironment.canUseDOM ? document.body : {appendChild: function() {}};

Choose a reason for hiding this comment

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

Why not going to the bottom of it and also modifying the way AppElement is resolved ? It would be great if document.body was never touched if a user wished so.


function getParentElement(parentSelector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally not so much of a fan of handling all these options here. Lately I've felt like maybe just going the function route is the best. That way the prop in all cases is just a function that returns a DOM node. Mixed examples don't have to be created and you don't have any issues with querySelector not returning what you expect it to, etc. Push the selecting of an element all out to userland rather than dealing with it in internal to the library.

if(typeof parentSelector === 'string') {
return document.querySelector(parentSelector);
}

if(typeof parentSelector === 'function') {
return parentSelector();
}

return parentSelector;
}

var Modal = React.createClass({

displayName: 'Modal',
@@ -35,32 +47,49 @@ var Modal = React.createClass({
onRequestClose: React.PropTypes.func,
closeTimeoutMS: React.PropTypes.number,
ariaHideApp: React.PropTypes.bool,
shouldCloseOnOverlayClick: React.PropTypes.bool
shouldCloseOnOverlayClick: React.PropTypes.bool,
parentSelector: React.PropTypes.oneOfType([
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be React.PropTypes.func now correct?

React.PropTypes.string,
React.PropTypes.func,
React.PropTypes.node
])
},

getDefaultProps: function () {
return {
isOpen: false,
ariaHideApp: true,
closeTimeoutMS: 0,
shouldCloseOnOverlayClick: true
shouldCloseOnOverlayClick: true,
parentSelector: document.body
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe specifying a function here would be best something like () => document.body would probably do.

};
},

componentDidMount: function() {
this.node = document.createElement('div');
this.node.className = 'ReactModalPortal';
document.body.appendChild(this.node);

var parent = getParentElement(this.props.parentSelector);
parent.appendChild(this.node);
this.renderPortal(this.props);
},

componentWillReceiveProps: function(newProps) {
var currentParent = getParentElement(this.props.parentSelector);
var newParent = getParentElement(newProps.parentSelector);

if(newParent !== currentParent) {
currentParent.removeChild(this.node);
newParent.appendChild(this.node);
}

this.renderPortal(newProps);
},

componentWillUnmount: function() {
ReactDOM.unmountComponentAtNode(this.node);
document.body.removeChild(this.node);
var parent = getParentElement(this.props.parentSelector);
parent.removeChild(this.node);
elementClass(document.body).remove('ReactModal__Body--open');
},