-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(Modal): handle init modal in SSR #1495
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defined isSSR
flag isn't reliable and window is still defined in multiple SSR environments for polyfills and backwards compatibility.
I believe you can drop that completely and just move this.init()
from construction into componentDidMount()
as this should work for all cases since it's not called in SSR render. @lamhieu-vk: Can you make this change and confirm this works in your testing for both SSR and client side rendering with open={true}
specified.
Reference:
Fix: #1071
Changes look good. @lamhieu-vk Is everything working as expected in your environment? |
@virgofx Sorry, I forget change title to |
No problem. Just let me know if everything is working in your environment as expected as I have not tested personally |
@virgofx I updated PR, I tested in my app and use |
Thanks. Looks good /cc @TheSharpieOne |
@virgofx you can squash and merge 😉 |
Just wanted you to look at it :) |
Many thanks for @virgofx @TheSharpieOne |
BTW, this PR also fixes #1323. |
@lamhieu-vk - Does your PR break auto-focus behavior? The implementation of Shouldn't that condition be changed to use componentDidMount() {
if (this.props.isOpen) {
this.init();
this.setState({ isOpen: true })
}
if (this.props.onEnter) {
this.props.onEnter();
}
// currently: if (this.state.isOpen && this.props.autoFocus) {
if (this.props.isOpen && this.props.autoFocus) {
this.setFocus();
}
this._isMounted = true;
} |
@justingrant I think your comment is true, and it's a break in this case. I will update it soon. |
In server-side rendering, Modal will call init but in this time not found
document
and it's will breaks.