Skip to content
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

Add a "reinstall" button when a block type is not found #22631

Merged
merged 6 commits into from
Sep 3, 2020

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented May 26, 2020

Description

If editing a post with a missing block, this PR adds the ability to reinstall it. It uses the Block Directory work to search for a block with the same name, and if found, gives you the option to reinstall the block. Once installed, the broken block is converted back into the original.

Fixes #24759

How has this been tested?

Only in-browser testing. I had a post with some missing blocks, one of which can be found on the block directory. You can copy this into the code editor to try it out (will install the Blissful Buttons plugin)

<!-- wp:sortabrilliant/blissful-buttons {"className":"is-style-offset"} -->
<div class="wp-block-sortabrilliant-blissful-buttons is-style-offset"><a class="wp-blissful-buttons__link" href="https://wordpress.org">This is a button.</a></div>
<!-- /wp:sortabrilliant/blissful-buttons -->

Screenshots

When first loaded, it searches for the block on wp.org. If no matching blocks are found, it only shows the Keep as HTML button (bottom). If it finds a block, it shows both Reinstall and Keep as HTML (top).

Screen Shot 2020-05-25 at 7 51 14 PM

Types of changes

New feature (non-breaking change which adds functionality)

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.

@ryelle ryelle added Needs Design Feedback Needs general design feedback. [Feature] Block Directory Related to the Block Directory, a repository of block plugins labels May 26, 2020
@ryelle ryelle self-assigned this May 26, 2020
@github-actions
Copy link

github-actions bot commented May 26, 2020

Size Change: +524 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-directory/index.js 8.5 kB +506 B (5%) 🔍
build/block-editor/style-rtl.css 10.8 kB +4 B (0%)
build/block-library/index.js 136 kB +14 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 126 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 8.55 kB 0 B
build/block-library/editor.css 8.55 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.47 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 742 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.7 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.6 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 12 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ryelle ryelle changed the title Block Directory: Add a "reinstall" button when a block type is not found Add a "reinstall" button when a block type is not found May 27, 2020
@StevenDufresne
Copy link
Contributor

I like this idea and I can see it becoming useful in a number of different "missing blocks" use cases.

if ( name !== 'core/missing' ) {
return settings;
}
settings.edit = missingInstallView;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine as a temporary approach. However, it could be very hard to maintain in the long run as you would have to keep the implementation in sync with changes applied to the Missing block.

Have you considered using an approach where you could parameterize action items in some way? It could be also done through filters but on the more granular level or through SlotFill approach.

@MichaelArestad
Copy link
Contributor

MichaelArestad commented Aug 18, 2020

This seems like a pretty valuable feature. I suspect the missing block notice may have been overlooked in the 5.5 design updates. Currently, it looks like this:

image

Perhaps we can fix up the padding and button styling in this or a follow up PR to bring it closer to the block placeholders:

image

I did modify the copy slightly:

Your site doesn’t include support for the "%s" block. You can try installing the block, convert it to a Custom HTML block, or remove it entirely.

Install "%s"

Keep as HTML

I wasn't able to get the button to appear locally. I tried with Layout Grid and Gutenslider (which I will not be installing again). Hamze has closed the Blissful Buttons plugin so I tested with Layout Grid.

I would like to see what happens when I click the install button. Is there an installing/activating state? Is it so fast it just magically works?

@MichaelArestad MichaelArestad removed the Needs Design Feedback Needs general design feedback. label Aug 18, 2020
@ryelle ryelle force-pushed the try/block-directory-install-missing-blocks branch from 22be2a0 to 1b15476 Compare August 25, 2020 22:10
@ryelle ryelle requested review from ajitbohra and Soean as code owners August 25, 2020 22:10
@ryelle
Copy link
Contributor Author

ryelle commented Aug 25, 2020

I've updated the code here to use a filter on the Missing edit view, so that it can fall back to the core missing component when blocks are not found or the block directory is not allowed. The initial view uses the default missing message, once the block directory returns a block it should load in the "Install [block name]" button.

Screen Shot 2020-08-25 at 6 07 09 PM

When clicked, it flips to a loading button while the plugin installs

Screen Shot 2020-08-25 at 6 07 23 PM

One thing that still needs work is that the attributes aren't carried over, for example in the above screenshots I've added a "Custom heading label", but when the Table of Contents block installs, it wipes out that content & is like a new block.

Copy link
Contributor

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good.

Can we get the button to left align with the paragraph content and add the same amount of padding/margin under the button?

/* translators: %s: block name */
__(
'Your site doesn’t include support for the "%s" block. You can try reinstalling the block, convert its content to a Custom HTML block, or remove it entirely.'
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this copy since it clarifies the 3 actions that i can take. However, we only show 2 of them below. I was expecting to see the remove action next to the "Keep as HTML" link.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current "missing" block uses the same pattern, so I'd rather leave it the same here.

Screen Shot 2020-08-26 at 10 45 07 AM

const hasContent = !! originalUndelimitedContent;
const hasHTMLBlock = getBlockType( 'core/html' );
const hasPermission = useSelect( ( select ) =>
select( 'core/block-directory' ).hasInstallBlocksPermission()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call delays the "Re-install" button's rendering. It's a bit jarring. Since the block directory uses "plugin install" permissions, is there a better way to get the permissions without having to do it async?

If not, maybe we should assume that the user can and let the endpoint manage it.
Related: https://github.com/WordPress/gutenberg/pull/16524/files#r315493053

<Button
onClick={ () =>
installBlockType( block ).then( ( success ) => {
if ( success ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the block I was testing with errored out because of an issue with its JS. The UI just stares back at you doing nothing.

You can test with Boxer Block.

index.js:94 Uncaught (in promise) TypeError: Cannot read property 'name' of undefined at Object.replaceBlock (index.js:94) at use-dispatch-with-map.js:68 at convertToBlock (index.js:44) at install-button.js:36

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More specifically, it installed properly but errored on injection.

@@ -0,0 +1,57 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected this file to only have one button, not sure if you named it this with the intention of it being included into core, where it probably would be only 1 button.

@ryelle
Copy link
Contributor Author

ryelle commented Aug 26, 2020

@StevenDufresne Something went weird in your review, it looks like GitHub let you review the old version of this PR. I think the current iteration fixes your other points.

@StevenDufresne
Copy link
Contributor

@StevenDufresne Something went weird in your review, it looks like GitHub let you review the old version of this PR. I think the current iteration fixes your other points.

I think i may have had added a comment to a review a while ago but never submitted the review. GitHub must hold on to those, and on my end, it looks like they have been submitted but they actually haven't.

@MichaelArestad
Copy link
Contributor

I updated the styles to match the mockup and other patterns (like the block placeholder state). In doing so, I noticed a couple things that were confusing to me. Here's a quick screencap of me disabling a block in order to view the warning:

2020-08-27 10 47 22

You'll notice a few things. Once I clicked the button to reinstall Layout Grid, I got a new layout grid essentially. If I then refresh the page, I get my old content back along with the "newer version of this document" warning at the top. Do you know why my original content inside Layout Grid wasn't immediately restored?

The other really minor bug is that the buttons change once the warning figures out there is a block it can install. Is there any way we can prevent that button change flash? I'm open to ideas. This is what I'm talking about:

2020-08-27 11 01 30

ryelle and others added 5 commits September 1, 2020 15:58
… the block placeholder state

* Removed font size declaration - was inconsistent with other patterns particularly because of the use of `em` units for padding
* changed padding on the warning and changed margin on the warning message
* changed the side the padding was on for warning buttons
@ryelle ryelle force-pushed the try/block-directory-install-missing-blocks branch from a289d1e to 2413b8c Compare September 1, 2020 21:26
@ryelle
Copy link
Contributor Author

ryelle commented Sep 1, 2020

The other really minor bug is that the buttons change once the warning figures out there is a block it can install. Is there any way we can prevent that button change flash?

It shows the current missing block view while we're searching the block directory for this block. We don't know if the block exists in the directory (or if it's some custom block), and we don't know the human-friendly name, until the API result comes back. We could add a spinner, or prevent showing any buttons while the search is running, use different copy? I'm not sure what would be best.

Do you know why my original content inside Layout Grid wasn't immediately restored?

Figured it out! It looks like nested blocks weren't being correctly updated, but they should be now.

@MichaelArestad
Copy link
Contributor

We could add a spinner

I think something like that could work. Let's shelve that for a future PR. This one looks good to go from a design standpoint.

@@ -3,8 +3,7 @@
display: flex;
flex-wrap: wrap;
font-family: $default-font;
font-size: $default-font-size;
padding: ($grid-unit-10 - $border-width - $border-width) $grid-unit-15;
padding: 1em;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having padding set as an em unit could lead to some odd outcomes should the block be broken inside of a parent block. It will inherit its font size. May be better to use rem units.

Copy link
Contributor

@MichaelArestad MichaelArestad Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an open PR to switch to em units and the block Placeholder state already uses em units. That's the reason for the change. Might as well be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up....

I wonder if that PR is considering third party blocks. This may get interesting. 🍿

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR looks like it was halting for further testing. That said, this will still be more consistent with the placeholder state for other blocks.

Copy link
Contributor

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minus my comment about using em units. This works as expected and is good to go.

As @MichaelArestad has mentioned, we will want to address the UI change when we find a block directory block in the future.

@ryelle ryelle merged commit d62d455 into master Sep 3, 2020
@ryelle ryelle deleted the try/block-directory-install-missing-blocks branch September 3, 2020 20:22
@github-actions github-actions bot added this to the Gutenberg 9.0 milestone Sep 3, 2020
@mcsf
Copy link
Contributor

mcsf commented Sep 7, 2020

I'm late to the party, but have you considered using the existing setUnregisteredTypeHandlerName interface to oust core/missing rather than introducing the editor.missingEdit filter? I'm finding it hard to justify the extra hook (it's fair to say that over time we've been favouring JS hooks less and less when the goal is to extend visual components — typically better solutions are offered either by Slot/Fill or by replacing components, as is the case with setUnregisteredTypeHandlerName).

@ryelle
Copy link
Contributor Author

ryelle commented Sep 7, 2020

Wouldn't setUnregisteredTypeHandlerName require me to rewrite all of core/missing again? It seems like I'd need to create an entire new missing block to set the handler to. Unless I'm misreading how setUnregisteredTypeHandlerName works?

I thought about a Slot/Fill approach, but that would probably look a lot like this filter, since I'd want to fall through to core/missing's view unless we find the block in the directory.

@youknowriad
Copy link
Contributor

If I'm understanding the issue, the requirements are:

  • Adding UI to the core missing block but doing it as a "plugin" to leave the "block directory" code on its own module (as a plugin).

--
Without introducing the new editor.missingEdit, can't we just hook into blocks.registerBlockType for the core/missing block and override the edit property?

@mcsf
Copy link
Contributor

mcsf commented Sep 8, 2020

Wouldn't setUnregisteredTypeHandlerName require me to rewrite all of core/missing again? It seems like I'd need to create an entire new missing block to set the handler to. Unless I'm misreading how setUnregisteredTypeHandlerName works?

I thought about a Slot/Fill approach, but that would probably look a lot like this filter, since I'd want to fall through to core/missing's view unless we find the block in the directory.

Yes to all your questions. :) I see the use case for allowing falling back to core/missing and wanting to avoid repetition. I still thought I'd ask, since filtering components can become more fragile over time if the decorated component changes. In this particular case, though, it's comforting to see that filterMissing is really just a filter — returning either an OriginalComponent instance or something entirely different. So I think a hook was a good call.

Without introducing the new editor.missingEdit, can't we just hook into blocks.registerBlockType for the core/missing block and override the edit property?

This does seem worth looking at.

@ryelle
Copy link
Contributor Author

ryelle commented Sep 8, 2020

Adding UI to the core missing block but doing it as a "plugin" to leave the "block directory" code on its own module (as a plugin).

Correct.

Without introducing the new editor.missingEdit, can't we just hook into blocks.registerBlockType for the core/missing block and override the edit property?

This is actually exactly what I did in the first pass of the PR, which @gziolo pointed out would be hard to keep in sync with any core core/missing changes (#22631 (comment)), though that was rewriting more of core/missing. I'll try out a PR for a more combined approach using blocks.registerBlockType.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins
Projects
None yet
6 participants