Block Directory: Add error messages inline.

[StevenDufresne]

This PR was originally here: #19589

I opened the changes here after foolishly rebasing + merging and mangling history.

Applicable comments were made here:
https://github.com/WordPress/gutenberg/pull/19589/files/2da9406051d66e0bea2fe8590d305db924da6002

---

Description

This PR is an experiment to add inline error messages to the block directory. This PR also introduces a change in execution that needs discussion.

Background Info

• We currently, download the assets (which means we inject scripts into the DOM)
• Then we install the assets by calling an API rest endpoint (.... the plugin ends up in their plugins folder)
• We currently execute download then install as a callback.
• During the download sequence, the block is registered which closes the menu (because the way the inserter works) and then the install is executed with the menu closed. Should any issues arise, the error message will appear with a bit of a delay.
• If the block fails at install we offer them a button and allow them to remove the block.
  • If they don't remove and reload the page, the block is broken. There is no way to fix it.

Types of changes

Changes:

Along with the obvious changes to add errors inline, this PR changes the order of execution. Instead of immediate injecting the assets in the download sequence and then installing. This PR first installs and then downloads.

Why?

• Before this PR, the inserter disappears as soon as the block gets registered in the download phase (the block registers and update the state). This means that if an error is throw on install, the context has already changed. We can't show anything inline, it's gone.
• If a block fails at install there isn't much recourse to fix it. Now, or potentially in the future. This means we have a broken plugin in the document. We therefore would need to have a robust "removal" experience. By flipping it, nothing is added until everything is :100 with the server.
• We could maintain a thinner front end package since the error logic and validation stays on the server and does creep to the front end.

Cons

• There is a delay as we install (subject to the network speed)
  • Added a loading state to the button to mitigate.
• ... probably more

Known Issues

• We should consider a better way to close the inserter. There is a callback that is passed in from the inserter slot onSelect, but it has a lower priority and depending on the state of the app, you could see a bit of a flicker. I have seen it sporadically.

Screenshots

	Before	After
Success
Install Error	Disregard that the component actually was loaded below.
Download Error		Mimicking this issue was tough and therefore I couldn't resolve the "Retry" work... but it is retrying :).

How has this been tested?

• Unit tests were added to cover the changes

Manual Testing

Manual test were done by doing the following: ( using these block: Card, Boxer, Listicle)

Case Success

1. Search/Add Block
   a. Notice that the block is added to the document without error
   b. Notice that the plugin is not in the regular WordPress plugin folder

Case Install Error:

1. Search/Add a block
   a. Make the /install endpoint return an error on first call
   b. Notice the error message is showing with the proper copy..
2. Click Retry
   a. Notice the error message is removed
   b. Notice the plugin is in WordPress plugin folder

Case Download Error:

1. Search/Add a block
   a. Add a throw statement in the download function in action.js to mimick and issue with adding assets to the DOM
   b. Notice the error message is showing with the proper copy.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[StevenDufresne]

Close up previews of errors:

Error on install

At this point, the code doesn't exactly know why.

Error on Download

We tried to inject the block but either the DOM was not ready, or the assets were not available.

[gziolo]

Interesting exploration. It looks slightly different than the design posted by @melchoyce where she described the proposed installation flow as follows:

Once the block is inserted into your content, WordPress would ideally start silently installing it in the background while you fill in placeholders and change settings. If you change your mind and decide this block doesn’t work for you, WordPress would deactivate and delete the block when you remove it from your content so you don’t end up with a junk drawer of discarded blocks in your library.

You can see the video presenting it in action in the Install section: https://make.wordpress.org/design/2019/06/07/block-library-mockups-prototype/.

I couldn't find any GitHub issue linked so I would be curious to learn what benefits do you see for this flow over the one posted by @melchoyce.

[StevenDufresne]

@gziolo Hey!

This experiment was initiated based on an item in this Issue:

• UI: Use inline notices (Follow-up work for the Block Directory #17440)

In order to use inline notices, we need to maintain the Inserter context and in that world, I would like to suggest we install then download. In a world where we don't use inline notices, the current installation sequence make sense.

[mapk]

@StevenDufresne this is all looking great. Having the error msgs inline with the popover work well.

My only hesitation here is the "Adding" button with the stripes. It's hard to tell, but does it follow the similar stripe pattern found in the "Publish" button?

Adding stripes:

Publish stripes

[StevenDufresne]

@mapk Thanks for the input.

My only hesitation here is the "Adding" button with the stripes. It's hard to tell, but does it follow the similar stripe pattern found in the "Publish" button?

Yeah, I was looking for the 'save' paradigm but it doesn't exist since everything to this point is instantaneous or quietly async. That is the only moment where the user is alerted that something is in progress.

Do you know of any save/loading examples in component/flows that have passed this test?

[mapk]

Do you know of any save/loading examples in component/flows that have passed this test?

I just realized that the "save" on reusable blocks does this very thing! It seems to use the same, or similar, UI as well.

[StevenDufresne]

Nice! The functionality is built into the Button component so we're probably going to see a lot more of it :).

[ryelle]

I've rebased & refactored this a bit:

• Disable the "Add Block" button when installing (also removes the border, which matches how the reusable block save button behaves)
• Update classNames to match standards
• Refactor install callback: This consolidates the install logic into one callback function, simplifying the list component
• Avoid children abstraction in ListItem, use DownloadableBlockNotice directly
• Get isLoading in DownloadableBlockHeader: This prevents some prop-drilling by getting the loading status when we need it
• Get the error notices in DownloadableBlockNotice: This prevents some prop-drilling by getting the notices when we need them

It seemed easier than going back and forth on reviews, but feel free to revert or discuss anything I did :)

For reference, this is what this PR currently looks like:

Click install, get the busy + disabled UI:

Oh no, an error:

[StevenDufresne]

@ryelle Changes looks good.

I fixed some broken tests and removed one. I'm not sure what the best way is to test whether the downloadAssets function is called or not in the way its architected. We could mock the action and check there. Feel free to add that test case.

[mapk]

I couldn't figure out how to get an error message, and the adding of the block happened so fast, I didn't see any loading UI. <- All that actually makes for a pretty good PR. Thanks y'all.

[ryelle]

I've got another PR in the works that further cleans up these actions & adds testing, so I think we could merge this one as-is, since it does have some test coverage. @StevenDufresne What do you think?

[StevenDufresne]

I've got another PR in the works that further cleans up these actions & adds testing, so I think we could merge this one as-is, since it does have some test coverage. @StevenDufresne What do you think?

I'm good with that.

[ryelle]

Looks good, @StevenDufresne you can merge this now 🙂 — the testing follow up PR is #22388