[fixed] Restore Modal.setAppElement() functionality #108
Conversation
| @@ -7,12 +7,15 @@ var elementClass = require('element-class'); | |||
| var renderSubtreeIntoContainer = require("react-dom").unstable_renderSubtreeIntoContainer; | |||
|
|
|||
| var SafeHTMLElement = ExecutionEnvironment.canUseDOM ? window.HTMLElement : {}; | |||
| var AppElement = document.body; | |||
There was a problem hiding this comment.
This would throw an error on Isomorphic/Universal applications :(
There was a problem hiding this comment.
I don't have experience with isomorphic apps, but I guess it's because document.body doesn't exist at compile time. Would it work if I do it like this instead?
var AppElement;
componentDidMount: function() {
...
(AppElement || document.body).appendChild(this.node);
...
},
There was a problem hiding this comment.
Seems there's a comment on every PR about something breaking when rendered on the server. Perhaps it's time for a simple unit test to catch these issues. The dev community is pretty equally split on those that write isomorphic apps and those who don't, as such we can't expect everyone that works on this to know that what they're doing is going to break something.
There was a problem hiding this comment.
var AppElement = ExecutionEnvironment.canUseDOM ? document.body : {appendChild: function() {}};That will provide safe usage for server side.
There was a problem hiding this comment.
Sorry, I don't understand. If AppElement === {appendChild: function() {}}, surely AppElement.appendChild(this.node) will do nothing. So the modal will not be appended to the DOM.
There was a problem hiding this comment.
@evoyy - yes, it would do nothing. That's the point on the server side, we're just creating a mock so that it doesn't try to access the DOM unless there's something there.
There was a problem hiding this comment.
Ah, of course. I will amend my PR. Thanks.
|
@jpserra Nice catch, thanks! |
|
Thanks for fixing this. Would love to see this get merged. 😃 |
|
Would love to get this on a release :D |
|
Same here, would love to have this merged! |
|
+1 for this PR! |
|
@evoyy This looks good, but could you squash the four commits down to one. It'll keep the git history a lot cleaner. Another great thing to have would be some specs testing things out. |
|
@claydiffrient Squashed and specs added. |
see react-modal >> [fixed] Restore Modal.setAppElement() functionality reactjs#108
Maybe I have misunderstood, but this commit will anchor the modal to the specified appElement, as I had expected it to from the README. It fixes #107.