-
Notifications
You must be signed in to change notification settings - Fork 8.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
avoid 'app not found' flickering while awaiting for mount #56483
avoid 'app not found' flickering while awaiting for mount #56483
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
if (!mounter || appStatus !== AppStatus.accessible) { | ||
return setAppNotFound(true); | ||
} | ||
setAppNotFound(false); | ||
|
||
if (mounter.unmountBeforeMounting) { | ||
unmount(); | ||
} |
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 main fix was to setAppNotFound(false);
before awaiting for mounter.mount
.
I also extracted all the non-mount related code outside of mount
function to execute all we can in a synchronous way (effects does not support async, so we are not effectively waiting for mount
to resolve)
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.
This is starting to feel like a class component would be simpler to understand.
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.
I agree. As discussed on slack, I think we should migrate back to class component next time we need to perform changing on this component. This may be when we'll want to show a proper loading message while awaiting the mounter.mount
result.
if (!mounter || appStatus !== AppStatus.accessible) { | ||
return setAppNotFound(true); | ||
} | ||
setAppNotFound(false); | ||
|
||
if (mounter.unmountBeforeMounting) { | ||
unmount(); | ||
} |
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.
This is starting to feel like a class component would be simpler to understand.
Tested locally, it worked. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
Fix #56409