Feature/modal mount node #1275
Feature/modal mount node #1275
Conversation
src/Modal.js
Outdated
| @@ -54,6 +54,7 @@ const propTypes = { | |||
| PropTypes.string, | |||
| PropTypes.func, | |||
| ]), | |||
| mountNode: PropTypes.any, | |||
There was a problem hiding this comment.
We should probably limit this like we do for the targets in tooltip/popover and allow for a DOMElement, function (which will return a DOMElement), or a string (which can be used to find a DOMElement).
There is a util which handles all of those cases, returning the DOMElement if found.
Probably should call it container to match similar functionality/props elsewhere.
There was a problem hiding this comment.
I initially had set the value to PropTypes.instanceOf(Element) but then decided to make it same as in Portal
Line 8 in 13e60ef
| @@ -235,6 +236,8 @@ class Modal extends React.Component { | |||
| } | |||
|
|
|||
| init() { | |||
| const mountNode = this.props.mountNode || document.body; | |||
There was a problem hiding this comment.
could probably use defaultProps to default it to document.body
There was a problem hiding this comment.
Look at the my first commit. I used defaultProps but when I run npm start it threw an error where document was undefined.
There was a problem hiding this comment.
Probably webpack-dev-server works in kind of SSR mode?
There was a problem hiding this comment.
Yeah, I suppose that will happen on the server when document is not defined. What you have here should be fine then.
|
This won't work with the forthcoming changes as well on server side. Everything in the init() will need to move to client side rendering as this breaks when {open} = true on SSR. Please do not merge this. |
|
Fixed in #1817 |
Relates to #1274
Should I add any unit tests? I'm not sure how to better test this feature