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 reusable blocks #2659

Closed
wants to merge 38 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@noisysocks
Copy link
Member

noisysocks commented Sep 4, 2017

Hello, reusable blocks!

panel-2

This PR adds all of the frontend code necessary to support viewing, creating, inserting, and editing reusable blocks.

#1516 describes the feature and #2081 (#2081 (comment), in particular) describes the technical approach I'm taking. We're using the backend API that #2503 added.

How to test

  1. With a Gutenblock selected, click 'Convert to reusable block' in the advanced options sidebar. This creates a new reusable block.
  2. Use the inserter to insert a new instance of this reusable block.
  3. With a reusable block selected in the editor, click 'Detach from Reusable Block' in the advanced options sidebar. This converts the reusable block back to a regular block.
  4. Play with creating, editing and saving different types of reusable blocks.

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch 6 times, most recently from 117296f to 7a85343 Sep 5, 2017

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Sep 8, 2017

I was talking with @nb and the topic of alternative approaches came up. Pasting my thoughts here, in case someone has any opinions on the matter. I'm pretty convinced that the redux approach that this PR takes is the right path, but I've been wrong before! 😄


The big alternative approach I’ve been mulling over is to enhance the Gutenberg blocks API to the point where reusable blocks could be added as a plain old block type that doesn’t need to touch redux at all.

To do this, the blocks API needs three things:

  1. Some way of letting blocks have internal state (think setState) that can be initialised from data that is passed into createBlock. This could be as simple as having the API let you mark an attribute as being private or non-serializable.
  2. Some way of letting you provide a higher order matching function instead of a blocks array when defining transforms.
  3. To pass the complete block in on calls to transform() instead of just the block attributes.

If we had these things then reusable blocks could, I think, look something like this:

registerBlockType( 'core/reusable-block', {
    ...
    attributes: {
        ref: { type: 'string' },
        _referencedBlock: { type: 'object' } // the underscore denotes that this is "private" 
    },
    transforms: {
        from: [
            {
                type: 'block',
                match: ( destinationBlock, sourceBlock ) => destinationBlock.name === sourceBlock.attributes._referencedBlock.name,
                transform( sourceBlock ) {
                    return createBlock( sourceBlock.attributes._referencedBlock.name, sourceBlock.attributes._referencedBlock.attributes );
                }
            }
        ],
        to: [
            {
                type: 'block',
                match: () => true,
                transform( sourceBlock ) {
                    return createBlock( 'core/reusable-block', {
                        ref: uuid(),
                        _referencedBlock: sourceBlock,
                    } );
                }
            }
        ],
    }
} );

Pros:

  • Feels nice having reusable block be just a block.
  • It's a lot less code.

Cons:

  • API changes feel a bit awkward and hacky.
  • Will likely need to use redux anyway to show reusable blocks in the Inserter.
  • Reusable blocks aren’t a native part of the editor—will make it hard to extend this feature in the future.

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from 7a85343 to 9a05107 Sep 8, 2017

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Sep 8, 2017

OK, most of the major pieces of reusable blocks are now implemented. I'd love to get some initial feedback on this PR before I start adding tests, cleaning it up, and breaking it up into smaller PRs! ❤️😃

Here's a lil' demo:

reusable-blocks

cc. @nb @mtias @westonruter @jasmussen

@munirkamal

This comment has been minimized.

Copy link
Contributor

munirkamal commented Sep 9, 2017

This looks great @noisysocks

If I am not missing, the possibility of making multiple blocks or a group of blocks reuseable is also on the roadmap after this?

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Sep 10, 2017

Good question, @munirkamal! I agree with @jasmussen's sentiments in #1516 (comment):

Now the mockups got me so excited, but what they in fact depict, should probably be for V2 of Gutenberg. And they should probably take advantage of block nesting (see #428). So you could imagine first inserting a "Group" block, and then moving a number of blocks inside that group. Once that is done, you can once again make that single group block into a reusable block.

@munirkamal

This comment has been minimized.

Copy link
Contributor

munirkamal commented Sep 11, 2017

@noisysocks

That makes sense, and I also agree with what @jasmussen suggested there.

Keep up the great work folks, Gutenberg is coming along really well.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Sep 26, 2017

Awesome work here!

Before digging into the code, I'd like to think a bit about the "modal" for the reusable block interactions and whether it's a good flow or not.

How does it work now:

  • You try to edit the block, and once the onChange is catched, we ask the user if he wants to update the reusable block or detach from the original block
  • If you detach, it's quite obvious, you'll be presented with a "local" block, and the reusable link is "lost"
  • if you updated the reusable block, the question is, when do we ask the user again if he want to continue editing the original block. For now, this is being done when remounting the component (for example moving to text mode and back to edit mode), but this is not ideal. A slightly better approach would be to persist this in local state and never ask until the user refreshes the page.

But still, I find this confusing.

I'd suggest the following flow if possible:

  • Preset the user with a read-only version of the block (calling the save function of the reusable block)
  • Show only two buttons in the block toolbar: Edit and Detach
  • Edit triggers the edit mode of the reusable blocks, showing a modal (or in place) the edit function of the reusable block and persisting this change when we "submit" it (we need a submit button in this edit mode)
  • Detach behaves like the current approach.

cc @mtias

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Sep 27, 2017

Thanks for your thoughts @youknowriad!

I agree that the modal is a little confusing. Something I've been noticing too is that it's especially confusing when there are multiple instances of the same reusable block in the post.

I like the toolbar idea—I'll spend some time experimenting with this approach! 😄

Would be good to get @jasmussen's opinion on this.

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from 86fd0b7 to 7d4b833 Sep 27, 2017

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Sep 28, 2017

Great work all around, and great thoughts too. I think the modals work fine, but I also think Riads suggestion is potentially better.

This is such a nice feature, it might be worth looking at it in two phases. One which features the modal as it is — seems like it might be worth getting this PR merged in and tested as is.

Then in phase two we can take the learnings from V1 and look at the UI improvements that Riad suggested. Unless of course there are technical underpinnings that would need this to change sooner rather than later, in which case I'd defer to Riad (and correct me if I'm wrong).

In both cases, we should do something about the visual appearance of the global block, if we can. Just like how collaborative editing has a colored border, we should have a distinguishing look to a block that is registered as global. It could, for example, have a dashed line instead of the solid line (and the way to do this as we learned from the collab editing stuff is add a CSS class to global blocks and assign the dashed border there).

I do agree with Riad that if the block appears to be completely like other blocks, editable and all, it's easy to forget that it's global, and the modal then becomes jarring. Perhaps an intermediate fix is to dim add a very subtle gray dimming overlay to the entire block (perhaps instead of, or in addition to the dashed border), change the cursor to a pointer instead of the caret. You'd then be able to click the block to get the "detach" prompt, instead of having this happen onChange.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Sep 28, 2017

Robert brought to my attention a problem with the idea suggested above:

I do agree with Riad that if the block appears to be completely like other blocks, editable and all, it's easy to forget that it's global, and the modal then becomes jarring. Perhaps an intermediate fix is to dim add a very subtle gray dimming overlay to the entire block (perhaps instead of, or in addition to the dashed border), change the cursor to a pointer instead of the caret. You'd then be able to click the block to get the "detach" prompt, instead of having this happen onChange.

In this situation you couldn't ever select a block (to move it or delete it, say), because that would trigger the prompt.

Which means we're back to either being fine with the onchange prompt for V1 and then making Riads suggested improvements for V2, or doing those changes before merging. I'm fine with either, but would defer to @karmatosed, @mtias or @youknowriad's feelings on this matter.

However you are able to edit a global block, it should still have a distinct style, seems like it's worth trying with a dashed line.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Sep 28, 2017

After further discussions with Robert, we mocked up this:

screen shot 2017-09-28 at 09 06 57

That's what happens when you select a reusable block — a panel at the bottom pops out at the bottom. When the block is just selected, it has "Edit" and "Detach" buttons, as well as a name there. When you've clicked "Edit", the name becomes editable, and there's a "Save" button there.

Thoughts?

Also, is that still a V2, or should that be V1?

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Sep 28, 2017

Also, is that still a V2, or should that be V1?

I'd be happy to do it for V1. The two approaches are much of a muchness, code wise. Removing the modal would feel kind of nice since it'd mean one less <Slot> in the app.

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from cee357b to a3cdd75 Oct 2, 2017

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Oct 2, 2017

I replaced the modals with the panel that @jasmussen and I discussed. Here's a GIF:

panel-2

Check out this video for a higher quality demonstration.

I'm really into it. Thoughts?

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 2, 2017

@noisysocks Nice, that looks good!

Question: are the "Edit" and "Detach" buttons as well as the title field hidden when the block is unselected? If so then 👍 👍 — see also https://github.com/WordPress/gutenberg/blob/master/docs/design.md#block-design-checklist-dos-and-donts-and-examples

Can you make it so the panel itself either has a background (imagine that quick doodle above had a gray background same as the border color), or a border separator above the fields? Just sort of separate content from chrome so to speak.

Does it work well on mobile? I.e. does it use available space in a good way? Remember you can use flexbox if you need the textfield to resize to available space. Buttons might be localized and grow bigger.

Given those tweaks I think we're ship shape to go!

@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Oct 2, 2017

@jasmussen this should be handled the same way in which we'll handle blocks that save to places outside of the post (i.e a site title block). Those blocks, at least in the post editor incarnation, would likely need their own "save" mechanism for clarity, so they should all do a similar thing.

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Oct 2, 2017

are the "Edit" and "Detach" buttons as well as the title field hidden when the block is unselected?

Yep! It looks similar to what happens at the end of my demonstration when I click the Detach button.

Can you make it so the panel itself either has a background

It has a light grey background right now. You can't see it in the low quality GIF, but if you watch the the video that I linked to, you'll spot it 😄

Does it work well on mobile?

Good point. I'll spend some time optimising it for small screens today.

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from a3cdd75 to 541f96f Oct 3, 2017

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Oct 3, 2017

Tweaked the styles a bit. Here's how it all looks now:

Desktop Mobile
vagrant local-wp-wp-admin-admin php-page gutenberg post_id 419 3 vagrant local-wp-wp-admin-admin php-page gutenberg post_id 419 iphone 6 2
vagrant local-wp-wp-admin-admin php-page gutenberg post_id 419 1 vagrant local-wp-wp-admin-admin php-page gutenberg post_id 419 iphone 6
vagrant local-wp-wp-admin-admin php-page gutenberg post_id 419 2 vagrant local-wp-wp-admin-admin php-page gutenberg post_id 419 iphone 6 1

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from 541f96f to cc53b55 Oct 3, 2017

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 3, 2017

I like those changes, nice work 👍 👍

@mtias as I'm still trying to grok the full extent of your comments above, can you chime in also? Thanks.

import './style.scss';

function ButtonControl( { instanceId, label, value, help, ...props } ) {
const id = 'inspector-button-control-' + instanceId;

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

Is the instanceId and the id useful in this component? Generally, we're using it to match labels and inputs but there's no input in this component.

This comment has been minimized.

@noisysocks

noisysocks Oct 3, 2017

Member

Good catch. <button> elements are labelable, but I wasn't passing id along to the <Button> component. I've fixed this in 224eb27.

// TODO: Can these two actions be combined?
// TODO: Think about a loading indicator, success message, failure message, etc.
this.props.updateReusableBlock( pickBy( { name, attributes } ) );
this.props.saveReusableBlock();

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

Yes, maybe the saveReusableBlockaction could take the updated name and attributes as arguments and optimistically update the state (similarly to how we do this for the savePost effect).

This comment has been minimized.

@noisysocks

noisysocks Oct 3, 2017

Member

The only problem is that saveReusableBlock is also dispatched when converting a static block into a reusable block. But maybe I could combine addReusableBlocks, updateReusableBlock, and saveReusableBlock into one action... 🤔

* TODO:
* These should use selectors and action creators that we gather from context.
* OR this entire component should move to `editor`, I can't decide.
*/

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

We're in the process of merging the two modules (blocks and editor) which will resolve this issue #2795

This comment has been minimized.

@noisysocks

noisysocks Oct 3, 2017

Member

Nice! I was dreading how awkward using context here would be! 😅

I'll leave the code as is until #2795 is merged.

This comment has been minimized.

@youknowriad

youknowriad Oct 12, 2017

Contributor

#2795 is closed. It was too disruptive for now but the Reusable block shows precisely that our current separation is not great @aduth


registerBlockType( 'core/reusable-block', {
title: __( 'Reusable Block' ),
category: null,

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

What does a null category mean? If I understand correctly, this hides the block from the inserter. this is probably just a side effect of how the inserter is built. Should we be more explicit and add a "private" flag instead?

This comment has been minimized.

@noisysocks

noisysocks Oct 3, 2017

Member

Yeah, that's right. I made it so that category can be marked null so as to have it not appear in the inserter.

A more explicit flag is a good idea. I've introduced this in ddc491b.

return $block_type->render( $block_attributes, $block_content );
} else {
return $block_content;
}

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

I wonder if we can factorize some code between here and the do_blocks function into a render_block( block ) function

This comment has been minimized.

@noisysocks

noisysocks Oct 4, 2017

Member

Great catch! Cleaned up in ccf3024 🌈

$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block_name );
if ( $block_type ) {
return $block_type->render( $block_attributes, $block_content );
} else {

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

Minor: the else is useless here.

This comment has been minimized.

@noisysocks

noisysocks Oct 3, 2017

Member

👌 removed in b1562b9.

dispatch( addReusableBlocks( reusableBlock ) );
dispatch( saveReusableBlock( reusableBlock.id ) );
dispatch( replaceBlocks( [ oldBlock.uid ], [ newBlock ] ) );
},

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

I'm confused, are these two actions reversed (attache/detach). Attach should create a reusable block and detach should create a regular block (as I understand it at least)

This comment has been minimized.

@noisysocks

noisysocks Oct 4, 2017

Member

Yeah, I wrote this all thinking that attach would create a regular block since you're taking a block that is unattached from the post (i.e., it's saved separately from the post) and attaching it to a post.

I was thinking of avoiding the issue altogether by making the actions MAKE_BLOCK_REUSABLE and MAKE_BLOCK_STATIC. Thoughts?

This comment has been minimized.

@youknowriad

youknowriad Oct 4, 2017

Contributor

I was thinking of avoiding the issue altogether by making the actions MAKE_BLOCK_REUSABLE and MAKE_BLOCK_STATIC. Thoughts?

sounds good to me 👍

This comment has been minimized.

@noisysocks

noisysocks Oct 5, 2017

Member

Fixed in f1368ba 📈

@@ -273,4 +284,106 @@ export default {

return effects;
},
// TODO: FETCH_REUSABLE_BLOCKS and FETCH_REUSABLE_BLOCK can probably be combined.
FETCH_REUSABLE_BLOCKS( action, store ) {

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

can't we replace all those custom fetching actions with the withApiData? Maybe not because we want these in the Redux state? in which case, maybe there's something we can do here. A wrapper to withApiData dealing with the state or something? @aduth any thoughts on this?

This comment has been minimized.

@noisysocks

noisysocks Oct 5, 2017

Member

Like you say, we'd need to change withApiData to store the models it fetches in Redux. It could be a cool abstraction. I feel it's best to punt on it for now, though.

This comment has been minimized.

@gziolo

gziolo Oct 5, 2017

Member

This is exactly what I'm looking for today :) It would be nice to be able to have an option to use the same API without a component, e.g. inside Redux middleware.

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from ccf3024 to 95840d1 Oct 4, 2017

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from ac0a295 to ec61310 Oct 12, 2017

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Oct 12, 2017

@aduth: Thanks so much reviewing! I've addressed or responded to your feedback.

I think the "save" button could have the same treatment we use for the "publish" button with the stripes. That signals something is happening. But the confirmation then happens via global notices.

@mtias: Great idea! I've implemented this in ec61310.

noisysocks added some commits Oct 1, 2017

Implement `render_callback` for reusable blocks
Allow reusable blocks to be rendered by the server for posts and pages
by registering them in PHP and defininig a `render_callback`.
Replace reusable block modals with inline edit panel
Replaces SaveConfirmationDialog, NewReusableBlockDialog, and Modal with
ReusableBlockEditPanel. This panel sits at the bottom of a reusable
block when selected and allows the user to explicitly edit and save the
reusable block. The end result is a less ambiguous and more explicit
UX.
Give the `<button>` in `<ButtonControl>` an `id`
This makes the `<label>` in the control behave correctly. According to
the spec, `<button>` is a labelable element:
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#Form_labelable
Use an explicit `isPrivate` flag to hide block types from the inserter
We don't wish to show the 'core/reusable-block' block type in the
inserter. We were doing this by taking advantage of the fact that the
inserter would ignore block types with a null category. This change
makes this behaviour more explicit.
DRY up `do_blocks` and `gutenberg_render_block_core_reusable_block`
Refactor the common logic in `do_blocks` and
`gutenberg_render_block_core_reusable_block` into a new
`gutenberg_render_block` function.
Combine FETCH_REUSABLE_BLOCK and FETCH_REUSABLE_BLOCKS
Simplify fetching logic a little by DRYing up these two mostly-similar effects.
Remove ADD_REUSABLE_BLOCKS action, add ability to track saving
Collapses the ADD_REUSABLE_BLOCKS action into UPDATE_REUSABLE_BLOCK and
FETCH_REUSABLE_BLOCKS_SUCCESS. Also makes it so that the Redux store
keeps track of whether or not a reusable block is being saved or has had
an error while saving.
Add reusable block 'saving' and 'save failure' UI
Adds a spinner that indicates whether or not a reusable block is saving,
and some UI that allows the user to retry the save if it fails.
Rename {ATTACH,DETACH}_BLOCK to MAKE_BLOCK_{STATIC,REUSABLE}
Avoid ambiguity around what 'attach' and 'detach' means by avoiding the
its use in code alltogether.
Improve tests for actions, selectors and effects
Slightly clean up the tests for reusable block actions and selectors,
and change our effects tests to reduce our dependency on mocking
selectors by taking advantage of the fact that MAKE_BLOCK_STATIC and
MAKE_BLOCK_REUSABLE are relatively pure functions.
Clean up <ButtonControl>
Remove unnecessary {}s, split up long line, and collapse SCSS selectors
into one line.
Clarify how we're using pickBy
Add a comment which clarifies this weird way we're using _.pickBy.
Call `blockType.save`, not `blockType.create`
Fix this typo. There's no such thing as `blockType.create`!
Use convertBlockTo{Static,Reusable} instead of makeBlock{Static,Reusa…
…ble}

It's a clearer action name since 'convert' is a less ambigious verb.
Use animation and notices to indicate the reusable block save state
- Makes the 'Save' button animate when a reusable block is saving
- Makes a 'Reusable block updated' notice appear when the save is
  successful
- Makes a 'Reusable block update failed' notice appear when the save
  fails

@noisysocks noisysocks force-pushed the noisysocks:add/reusable-blocks branch from ec61310 to 5637827 Oct 12, 2017

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Oct 13, 2017

Thanks for all the feedback, everyone! 🙏

I'm closing this and splitting up the work into three smaller PRs. The first is right here: #3017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment