-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Support reusable blocks for multi-selection #9732
Conversation
It is with great pleasure and honor that I present to you this HOLY GUACAMOLE THIS IS INSANE award for Oustanding Pull Request. This award comes with a lifetime supply of Kudos from me (non-transferrable), and is only given out once per year and only when the situation warrants this. This is amazing, amazing work. GIF: 👏👏👏Every bit of feedback I have here is minuscule, overall this PR is just so damn cool. Honestly I didn't think it could work — I thought we needed the section block to exist first, so you could use that to group things. But this is working better than I'd expected. It's essentially a blueprint, all we need is a way to share it. How hard would it be to create a block menu option that says "Export Blueprint", which gave you a JSON string you could copy paste into another Gutenberg editor? Anyway I'm getting ahead of myself. There are some very small issues we can address. Not sure they need to be part of this PR or separate, and I would love to help you with them, just let me know:
Holy wow, imagine this: wp-blueprints.com, where people can group blocks together and share their JSON strings with nice little copy buttons. Categories for top rated, most downloaded, search, etc? This is going to happen because of this magic (⊃。•́‿•̀。)⊃━☆゚.*・。゚ ƪ(˘⌣˘)┐ ƪ(˘⌣˘)ʃ ┌(˘⌣˘)ʃ |
@jasmussen ❤️
This was one of my goals for opening this PR. You know iterations ;)
I'm fine either way, maybe we should just do it separately for the sake of separating concerns as it's not directly related to this PR.
I'm leaning towards "pretty hard" as I have no idea how to start here :P |
Alright, I can take this one. Want me to push directly to this branch? |
Yes definitely 🎉 |
Okay, so I know how to fix it the issue with wide nested blocks. Essentially, if a reusable group (should that be the name?) has children that are wide or fullwide, then the reusable block also needs to be wide or fullwide, or at least aware of it. This is what happened if I simply added the Before:
After:
Note, I'm not suggesting we do this. In fact the reusable blocks UI is not fully usable in this state. But to explain what's going on — imagine you created a Columns block with 1 column (not possible but pretend it is), and added a fullwide image inside. Unless the columns block was also fullwide, then the fullwide image inside would be limited by the width of the columns block. Reusable groups of blocks is the same, it basically seems to add a wrapper around the blocks you multiselect, and that wrapper is born with a max-width same as all other non-wide blocks. There are a couple of ways we can solve this:
2 is probably cleanest, because it is pretty simple. Something like this would almost be enough to fix the issue:
That would make it look like this: As you can see, there's still some side UI stuff that we'd want to take a look at (in fact I might just apply the same styling to reusable block groups as we do to fullwide blocks). But that should be fairly easy to build. Can you make such a CSS class for me? |
@jasmussen I tried what you propose (2nd option) but the issue is that it makes regular reusable blocks look wide even if they are not |
Hmmmmm even with no change to the existing is-reusable class? Or can you elaborate a bit on it? Do you mean if you group two text blocks they look wide? |
This is amazing! I know this isn't happening but please push this with 3.8 :) |
Unfortunately, the RC for 3.8 is already published, so this will be happening in 3.9 probably. |
I can live with that, anyways, great pr! |
Wow, this is really cool. I can see this being used for all sorts of things like templates for CTAs or other page building elements or even entire pages, without needing to use a containing parent block of some sort. This kind of thing could allow for choosing a page template when you create a new page. No need to create a new system for that when you can just reuse the Reusable blocks library! Reusable blocks can now also be reusable templates! I think that this would be a big step forward for Gutenberg. It makes Reusable blocks considerably more powerful.
Actually, this is completely possible. You just have to manually edit the columns number input. |
@youknowriad Can you add the following icon to any multi-select reusable blocks users create?
|
Sure @jasmussen added. |
block-library/index.js
Outdated
@@ -36,6 +36,7 @@ import * as nextpage from '../packages/block-library/src/nextpage'; | |||
import * as preformatted from '../packages/block-library/src/preformatted'; | |||
import * as pullquote from '../packages/block-library/src/pullquote'; | |||
import * as reusableBlock from '../packages/block-library/src/block'; | |||
import * as reusableTemplate from '../packages/block-library/src/template'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we should call this just "template" here?
}, | ||
|
||
edit() { | ||
return <InnerBlocks />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the simplicity of this edit
function :)
@@ -223,7 +223,7 @@ describe( 'full post content fixture', () => { | |||
.map( ( block ) => block.name ) | |||
// We don't want tests for each oembed provider, which all have the same | |||
// `save` functions and attributes. | |||
.filter( ( name ) => name.indexOf( 'core-embed' ) !== 0 ) | |||
.filter( ( name ) => name.indexOf( 'core-embed' ) !== 0 && name !== 'core/template' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update the comment here too? Not clear why we need to exclude it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to add fixtures for this block. I thought this test would bring too much compared to the e2e test
This is looking good to me. Left a few nitpicks. |
@@ -691,14 +691,14 @@ export function convertBlockToStatic( clientId ) { | |||
/** | |||
* Returns an action object used to convert a static block into a reusable block. | |||
* | |||
* @param {string} clientId The client ID of the block to detach. | |||
* @param {string} clientIds The client IDs of the block to detach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed running docs update. Local changes on master after running npm run docs:build
Makes sense. To clarify: is the idea that we'll eventually deprecate edit: Oh! I see, it's never saved to the post's content. Clever! 👍 |
This PR adds the possibility to save a multiselection as a reusable block. To do so we create a hidden block called
core/template
serving as a container for the multi-selection and when converting back to a regular block, we drop thecore/template
container.Testing instructions
Notes
The
core/template
block is a temporary block never saved to post content or to thewp_block
CPT. After we land #7453 we will be able to get rid of it completely.Right now the assumption in the state is that a reusable block holds a unique block. We don't make this assumption server side and we will be able to get rid of this assumption after #7453 as well.