-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
MediaUpload: Keep track of the dialog state #62108
Conversation
if ( onClose ) { | ||
onClose(); | ||
} | ||
|
||
this.isModalOpen = false; |
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'm no Media Library code expert, but I think we should call this.frame.detach()
on close action. Otherwise, the modal leaves some "breadcrumbs" in the DOM. Core does a similar thing for classic widgets.
Screenshot
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.
Left a longer reply in Slack. TL;DR I agree.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +21 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
05e727a
to
706730e
Compare
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.
Have you considered a mount effect in the render function?
Or perhaps the component should open the modal if no |
What do you mean? I am not sure if it's worth encouraging the hack that the "Inline Image" is using. All other React class component methods are run twice in StrictMode, so we still have to track the dialog state even if it's open on |
|
It will have the same issue. The hooks also run one extra time in development mode, so the Is there a reason we want to avoid keeping the dialog state tracked in the component? |
Great ✅ |
Thanks for the feedback! 🙇 |
What?
Fixes #62105.
Related #61943.
PR updates
MediaUpload
to keep track of the Media Library dialog state. In rare cases, when theopen
method is called multiple times, it prevents several dialogs from opening.Why?
The "Inline Image" format opens the Media Library dialog by calling the
open
method during the render. It is a hack to emulate modal opening on a click, but React will call the class component methods like render twice in development.Testing Instructions
Patch for enabling modes
Testing Instructions for Keyboard
Same.