Don't construct ModalManager while document.body may still be null #4982
Conversation
|
this isn't semantically correct; we need the modal manager to be shared |
|
@taion Could you please elaborate? What do you mean by shared? And what isn't semantically correct? Thanks for your feedback! |
|
By default, all instances should use the same modal manager. Essentially, you need logic like https://github.com/react-bootstrap/react-overlays/blob/e022e9c2b66fb7a0f8ef0c172a1b12c986dbcc75/src/Modal.js#L316-L326 to make sure this happens. |
|
I see you point. We could also do it like that, as long as it's only going to instantiate the I'll update the PR accordingly. |
xijo
force-pushed
the
attempt_to_fix_document_body_missing_during_modal_import
branch
2 times, most recently
from
30e5541
to
5372504
Compare
|
PR is updated, please have another look. Thanks 👍 |
src/Modal.js
Outdated
| modalContext = { | ||
| onHide: () => this.props.onHide(), | ||
| }; | ||
|
|
||
| constructor(props) { | ||
| super(props); | ||
| manager = manager || props.manager || new BootstrapModalManager(); |
There was a problem hiding this comment.
you need to bind the global manager to the newly constructed manager, as in the R-O code, so that additional modals use the same manager
There was a problem hiding this comment.
I believe, since the assignment refers to the global variable, this should work as expected.
However, I updated the PR to fit the style of R-O.
|
please just take a look at the logic in react-overlays and use that here. that logic does the right thing. |
xijo
force-pushed
the
attempt_to_fix_document_body_missing_during_modal_import
branch
from
5372504
to
87b89aa
Compare
|
thanks! |
xijo
deleted the
attempt_to_fix_document_body_missing_during_modal_import
branch
Hello everyone,
This PR is referring to the following issue #3645 which is not yet fixed.
If
ModalManageris constructed withdefaultPropsthere is still a chance thatdocument.bodyis not yet present.By delaying the construction until the constructor of the
Modalis actually called, we can avoid this problem.I know that one can avoid this problem by moving all JS includes to the body, but that can be impractical and weird in larger project, where you gotta keep track of all the includes that are being loaded.
Best,
Jo