Block Directory: Add error messages inline. by StevenDufresne · Pull Request #20001 · WordPress/gutenberg

StevenDufresne · 2020-02-03T04:23:56Z

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

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:

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..
Click Retry
a. Notice the error message is removed
b. Notice the plugin is in WordPress plugin folder

Case Download Error:

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 · 2020-02-03T05:12:02Z

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 · 2020-02-03T07:00:57Z

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 · 2020-02-03T07:55:15Z

@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 · 2020-03-07T00:02:28Z

@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 · 2020-03-09T05:36:20Z

@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 · 2020-04-14T02:09:35Z

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 · 2020-04-14T05:08:32Z

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

ryelle · 2020-05-08T18:57:24Z

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 · 2020-05-11T01:47:39Z

@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 · 2020-05-11T23:51:49Z

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 · 2020-05-14T22:26:48Z

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 · 2020-05-15T06:19:06Z

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

This prevents some prop-drilling by getting the notices when we need them

This prevents some prop-drilling by getting the loading status when we need it

This consolidates the install logic into one callback function, simplifying the list component

StevenDufresne mentioned this pull request Feb 3, 2020

Block Directory: Try/block directory inline error msgs. #19589

Closed

6 tasks

StevenDufresne requested a review from noisysocks February 3, 2020 04:52

gziolo added Needs Design Feedback Needs general design feedback. [Feature] Block Directory Related to the Block Directory, a repository of block plugins labels Feb 3, 2020

gziolo requested review from mapk, melchoyce and tellyworth February 3, 2020 06:40

gziolo requested a review from jorgefilipecosta February 3, 2020 07:10

StevenDufresne mentioned this pull request Apr 6, 2020

Add script and style REST APIs #21244

Open

6 tasks

ryelle force-pushed the try/block-directory-inline-error-messages branch from 898a711 to 715eedf Compare May 8, 2020 18:50

ryelle mentioned this pull request May 15, 2020

Block Directory: Store refactor #22388

Merged

3 tasks

ryelle force-pushed the try/block-directory-inline-error-messages branch from 7fea9d1 to ca0f8a6 Compare May 15, 2020 18:58

ryelle approved these changes May 15, 2020

View reviewed changes

StevenDufresne and others added 5 commits May 18, 2020 09:50

Block Directory: Add error messages inline.

2857e99

Make the linter happy

32d370b

Place the retry button on its own line.

2b40e0c

Update classNames to match standards

8a14e44

Get the error notices in DownloadableBlockNotice

634dec8

This prevents some prop-drilling by getting the notices when we need them

ryelle and others added 6 commits May 18, 2020 09:50

Get isLoading in DownloadableBlockHeader

e9ab77e

This prevents some prop-drilling by getting the loading status when we need it

Avoid children abstraction in item

a7a5a5e

Refactor install callback

1dc609b

This consolidates the install logic into one callback function, simplifying the list component

Disable the "Add Block" button when installing

9f02f68

Fix broken tests.

62fbce6

Update padding under error messages.

30a33e2

StevenDufresne force-pushed the try/block-directory-inline-error-messages branch from ab91c99 to 30a33e2 Compare May 18, 2020 00:51

StevenDufresne merged commit 573767e into WordPress:master May 18, 2020

github-actions bot added this to the Gutenberg 8.2 milestone May 18, 2020

Conversation

StevenDufresne commented Feb 3, 2020

Description

Background Info

Types of changes

Changes:

Why?

Cons

Known Issues

Screenshots

How has this been tested?

Manual Testing

Case Success

Case Install Error:

Case Download Error:

Checklist:

Uh oh!

StevenDufresne commented Feb 3, 2020

Error on install

Error on Download

Uh oh!

gziolo commented Feb 3, 2020

Uh oh!

StevenDufresne commented Feb 3, 2020

Uh oh!

mapk commented Mar 7, 2020

Uh oh!

StevenDufresne commented Mar 9, 2020

Uh oh!

mapk commented Apr 14, 2020

Uh oh!

StevenDufresne commented Apr 14, 2020

Uh oh!

ryelle commented May 8, 2020

Uh oh!

StevenDufresne commented May 11, 2020

Uh oh!

mapk commented May 11, 2020

Uh oh!

ryelle commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StevenDufresne commented May 15, 2020

Uh oh!

ryelle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryelle commented May 14, 2020 •

edited

Loading

ryelle left a comment •

edited

Loading