Image: Display errors after failed upload #39178
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
commented
Mar 3, 2022
Mamaduka
added
[Block] Image
|
Size Change: +333 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
I tested this and it works if just setting the default error message so every image fails, but in the gallery if an image really fails due to exceeding the php.ini limit then it goes back to the endless spinner: With every image getting fake error: With php.ini upload_max_filesize set to 1MB, so one image getting a |
|
Adding back the |
|
Thanks for testing, @glendaviesnz. It's interesting that we get different behavior for emulated and actual errors. I reverted the |
Mamaduka
commented
Mar 4, 2022
| @@ -171,6 +171,7 @@ export function ImageEdit( { | |||
| function onUploadError( message ) { | |||
| noticeOperations.removeAllNotices(); | |||
| noticeOperations.createErrorNotice( message ); | |||
| setTemporaryURL( undefined ); | |||
There was a problem hiding this comment.
I realized that we only need to reset the state. The onSelectImage will remove attributes for us. It's called during the error but with an undefined media argument.
There was a problem hiding this comment.
This is a weird one 🤔 - for me this change causes the php.ini upload limit error to be ignored, but only in the gallery:
img-error.mp4
Seems like there are a couple of gotchas with how gallery versus individual images blocks are behaving with these errors, which would be good to work out why and resolve, or at least document. I will try and get a closer look at it later today.
There was a problem hiding this comment.
I had another look at it, and for some reason it seems like the standalone Image block goes through an extra render cycle which causes image to be removed and error to display, but this doesn't happen if the Image block is in a Gallery, but I can't work out why, as the Gallery essentially just delegates the upload process to the Image block. I will take another look at it tomorrow if you don't have any ideas.
We could just put the setAttributes back, but it would be nice to know why this is happening.
There was a problem hiding this comment.
Thanks for testing, @glendaviesnz. I will revert setAttributes removal.
I would love to refactor the whole Image block at some point - the |
glendaviesnz
approved these changes
Mar 7, 2022
|
Thanks for the reviews, @glendaviesnz 🙇 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #39116
PR ixes the issue when the upload error message wasn't displayed. Also re-uses
onUploadErrorcallback for error messages.Testing Instructions
Example
Screenshots
Before
CleanShot.2022-03-03.at.09.09.44.mp4
After
CleanShot.2022-03-03.at.09.15.09.mp4
Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).