-
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
Post publish upload media dialog: fix silent failure #64741
Conversation
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: +25 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
Thanks, @sgomes! This only partially solves the issue. If fetch fails with a Example blocks
ScreencastCleanShot.2024-08-23.at.16.46.10.mp4 |
Thank you for taking a look, @Mamaduka! 🙌
Yes, we should fix that as well 👍 But to clarify, this PR is intended to fix a bug in the updating of the blocks, assuming that the upload works correctly. What do you think would be the best way to handle errors in the upload process? Displaying an error message and changing the button text to "Retry"? Also, would it be best to include those changes in this PR, or to open a new one for them? |
I think that should work. @jameskoster or @jasmussen might have better suggestions :)
Yeah, it can be done in follow-up PR. Don't want to block this. I think we could fix one minor thing here. If you watch the screencast closely, you'll see that the "Upload" button becomes active for a second before removing the uploaded image from the list. This can result in duplicate uploads on some devices if users click it again. Maybe we should optimistically remove uploaded images. What do you think? |
Perhaps the simplest way to start is to allow the "Upload" button to remai as-is, but indeed output, to the right of it, or below it, "Upload failed, try again." |
Thank you again, @Mamaduka! I added some code to wait until the animation ends before showing the button again: Wait.for.animation.movI'll follow up with the UI changes for handling upload errors in a new PR. |
105a9d6
to
aa2188e
Compare
I started getting a new error when testing this, in a portion of the code that hasn't changed. Please hold off on re-testing while I investigate. |
False alarm, the error was due to a local issue in my machine. Please take another look when you get a chance! |
Thank you again for the review, @Mamaduka! 👍 @jasmussen : Thank you for the suggestion, I'll open a new PR 👍 |
* Post publish upload media dialog: fix silent failure * Wait until animation ends before activating button
What?
Attempt to fix intermittent silent failures in the post publish upload media dialog.
Why?
The upload process sometimes silently fails when using the upload media dialog:
Upload.fails.mov
The files do get uploaded, but the blocks never get their attributes updated, causing the upload suggestion to remain.
When debugging this, I noticed that the
clientId
s that are being updated don't seem to exist in the block editor store. My knowledge is extremely limited here, but I believe this may be due to a mismatch inclientId
s between the editor store and the block editor store.This PR may fix #63956.
How?
This PR switches from using the editor store
getEditorBlocks()
selector to the block editor storegetBlocks()
selector. This seems like a reasonable fix, given that theupdateBlockAttributes()
action that we use to update blocks belongs to the block editor store, but again, my knowledge is extremely limited here.Testing Instructions
I don't know how to reliably reproduce the failure state 😞 It sometimes happens when using patterns that include image blocks, and which point to theme images (which are considered external images).I figured out how to reproduce the failure state:
https://s.w.org/style/images/about/WordPress-logotype-simplified.png
)This should produce a similar failure to the one in the video above before applying this PR. The same sequence of steps should work correctly after the PR.
Testing Instructions for Keyboard
N/A
Screenshots or screencast