Add an error state to the image block to allow upload errors to display

[brentswisher]

Description

Adds an error state to the image block to display errors that have occurred when a file fails to upload. This is an issue when a file is dropped into the editor or onto an empty image block, so the media library modal is not used.

Uses the existing noticeOperations methods for creating and displaying the errors passed back from the mediaUpload component.

Fixes #10191 as well a similar issue I discovered when an image of an improper format (.psd) was used.

How has this been tested?

Open the editor and try to drag an image that is larger than the max file upload limit.
The error message passed back from the mediaUpload component should now be displayed in an empty image block.

Types of changes

Adds a hasError variable to the state of the image block component.
Sets this state based on the mediaUpload component passing back an error.
If set to true, use noticeOperations to get and display the error messages.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[jorgefilipecosta]

I think if make a call here similar to:

						noticeOperations.createErrorNotice( message );
						this.props.setAttributes( {
							url: undefined,
							alt: undefined,
							id: undefined,
							caption: undefined,
						} );

We may be able to avoid the need for a state flag.

[jorgefilipecosta]

Hi @brentswisher thank you a lot for your contribution, things worked great in my tests 👍
I left a possible suggestion for an improvement that may remove the need for a flag in the state.
Feel free to ping me if you need additional information!

[brentswisher]

@jorgefilipecosta thanks for the feedback! I tried it the way you had suggested and that also displays the error, but there was a noticeable delay before the error message showed up. It seems almost like the file was complete uploaded before displaying the error? With the flag in the state, it shows the error almost instantaneously, which seems like maybe a better user experience.

I don't entirely understand why your suggestion should slow the error displaying, so maybe I am doing something wrong. I modeled the hasError flag off of @noisysocks implementation of the file block, maybe he would have some insight?

[brentswisher]

I'm on a bad connection right now but can upload some screen captures of the difference in error timing tomorrow.

[brentswisher]

In looking into it a little bit more last night, it looks like the url gets set to a blob on the start of upload, and updated as the upload processes? I'm thinking it doesn't get set to undefined and stay until after it is finished uplaoding. With the state variable, it displays the error while the file is still getting uploaded, because it doesn't care what the url is set to. Again, very new to the gutenberg code so feel free to correct if I am mistaken.

Here is the upload process without the state variable:

Here is the process with the state variable:

[jorgefilipecosta]

This seems a complicated problem. If we apply a setAttributes here, the attributes are changed in the store, but the components are not rerendered even if caching is disabled on the getBlocks selector. That's why there is a delay on the notice appearing when we use the setAttributes approach.

@noisysocks given that the state approach was also used on the file blocks do you have any insights on why setAttributes does not work for this cases?
I'm fine with the approach being proposed here my only fear is that the fact the setAttributes approach does not work well may be related to some problem that may have an impact in other areas.

Thank you @brentswisher for this PR and for the research you made.

[noisysocks]

We opted to use setState() for the File block because using setAttributes() causes a useless undo level to be created.

[brentswisher]

The image block has changed a bit since I wrote the original bug fix but the error is still occurring. I merged master back into this branch so it is able to be merged and updated the code to simplify it a little bit. Since @talldan and @imath added the isEditing bit to the state in #9264, I rewrote my hasError addition to use that instead, does that clear up your concerns about this @jorgefilipecosta ?

[talldan]

Thanks for the contribution @brentswisher. 😄

I gave it a test and confirmed it works. This is also consistent with the file block, so that's nice.

[talldan]

I liked the whitespace! 😢

[brentswisher]

To be honest, it was unintentional but didn't see it till it was already committed, I can patch to add it back?

[talldan]

Ah, ok! It's not the end of the world, but might be good to add it back if it was unintentional. There were a few others removed as well.

[brentswisher]

Not a problem it should be all set now - thanks for pointing it out

[brentswisher]

@talldan Replacing that whitespace triggered Travis CI and it failed for some unrelated reason 🤦‍♂️
Is that something you can restart, or should I ask somewhere else?

[talldan]

Hi @brentswisher - there were some minor issues after the 5.0 release with the CI jobs. Everything now seems to be passing so will merge this 👍

[brentswisher]

Awesome, thanks!