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

[WIP] Child blocks #6346

Closed
wants to merge 4 commits into from
Closed

[WIP] Child blocks #6346

wants to merge 4 commits into from

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Apr 23, 2018

What this is 🧐

WIP attempt at implementing #5540.

The meat of these changes is a new allowedParents property which lets you specify which blocks a child is allowed to be nested into:

registerBlockType( 'acme/buy-button', {
	title: 'Buy Button',
	icon: 'cart',
	category: 'common',

	allowedParents: [ 'acme/product' ],
	...
} );

See the test plugin for a full code example. I reccomend installing the plugin, checking out this branch, and playing around with the API.

Implementation details 🤓

Block registration API changes

  • Introduces allowedParents which lets one specify which block types and post types a block can be nested into.
  • Deprecates isPrivate. Instead, use allowedParents: false.

Selector changes

  • getInserterItems now respects the above properties and is the source of truth for whether a block is insertable or not.
  • Items returned by getInserterItems will include new utility and frecency attributes. utility indicates how useful we think inserting the block will be. This lets us show contextually useful blocks at the top of the Suggested tab.
  • Deprecates getSupportedBlockTypes. getInserterItems will remove any blocks that are not supported.
  • Deprecates getFrecentInserterItems. Instead, call getInserterItems and filter/sort by frecency.

Inserter changes

  • Both inserter and inserter-with-shortcuts have been changed to use getInserterItems and its new functionality.

How I've tested this ✅

I wrote a little test plugin which defines a custom post type (Store) and two blocks (Product & Buy Button).

If you install the plugin, you can see that:

  1. Product can only be inserted into a Store CPT.
  2. Buy Button can only be inserted into a Product.
    1. This occurs even when allowedBlocks does not contain Buy Button, because allowedParents takes precedence.
  3. When inserting Buy Button, the block always appears at the top of the inserter.
    1. This occurs even when allowedBlocks is set to true.
  4. The inserter behaves like it always has.

Outstanding questions 🤔

  1. Is this API flexible enough for plugin developers?
  2. Should getInserterItems be a selector?
    Pro: It's technically a pure function that takes state as an input.
    Pro: A selector means that this API is available to plugin developers.
    Con: It's way more complicated than a regular selector.

Still to do 👨‍💻

  • Write a test plugin
  • Fix unit tests for getInserterItems
  • Improve getInserterItems performance (e.g. by using createSelector)
  • Write documentation for new API

@noisysocks
Copy link
Member Author

noisysocks commented Apr 24, 2018

@gziolo @mtias What do you think of this API for Child Blocks? It's essentially what was proposed in #5540 (comment). Check out this gist for an example of a plugin that uses it.

@noisysocks noisysocks added [Status] In Progress Tracking issues with work in progress [Feature] Extensibility The ability to extend blocks or the editing experience labels Apr 24, 2018
@jasonbahl
Copy link

This looks awesome! This currently missing functionality is a pretty big blocker (pun intended) for using Gutenberg in production projects.

@noisysocks
Copy link
Member Author

noisysocks commented May 4, 2018

I've updated this PR to use the API I described in #5540 (comment). Check out this gist for a few different examples of its usage. I think it works quite nicely. Any thoughts before I add documentation and tests?

@gziolo @mtias @jorgefilipecosta

@noisysocks noisysocks force-pushed the try/child-blocks branch 2 times, most recently from ec6f5e9 to eb68f34 Compare May 4, 2018 06:18
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I think that there is a general agreement that it should be implemented as you proposed even though it might be confusing in some cases. On the technical side I like where this is going and would love to see it polished.

*
* @return {string[]|boolean} Blocks that can be nested inside the block with the specified uid, or true/false to enable/disable all types.
*/
export function getSupportedBlocks( state, uid, globallyEnabledBlockTypes ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should it also be deprecated for some time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes—good catch 😄

@noisysocks
Copy link
Member Author

Thanks @gziolo. I'll get this ready to ship!

Block registration API changes:

- Introduces `allowedParents` which lets one specify which block types
  and post types a block can be nested into.
- Deprecates `isPrivate`. Instead, use `allowedParents: false`.

Selector changes:

- `getInserterItems` now respects the above properties and is the source
  of truth for whether a block is insertable or not.
- Items returned by `getInserterItems` will include new `utility` and
  `frecency` attributes. `utility` indicates how useful we think inserting
  the block will be. This lets us show contextually useful blocks at the
  top of the Suggested tab.
- Deprecates `getSupportedBlockTypes`. `getInserterItems` will remove
  any blocks that are not supported.
- Deprecates `getFrecentInserterItems`. Instead, call `getInserterItems`
  and filter/sort by `frecency`.

Inserter changes:

- Both `inserter` and `inserter-with-shortcuts` have been changed to use
  `getInserterItems` and its new functionality.
Use createSelector() to cache the results of getInserterItems() and
increase inserter performance.
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I have some issues with immediately understanding the code that are probably related to my being new to the project. Maybe just some comments as clarification would help there.

Beyond that, I think the list arguments are too fancy for little benefit. It means various blocks will use a slightly different API for declaring Allowed Parent Block Types making it hard to easily work on different blocks depending on who coded it and which API they prefer. I think it could use simple JS objects with '*': true and '*': false` as special values, rather than the much more flexible and less consistent argument types it takes now.

@@ -173,7 +173,7 @@ export const name = 'core/block';
export const settings = {
title: __( 'Shared Block' ),
category: 'shared',
isPrivate: true,
allowedParents: false,
Copy link
Member

Choose a reason for hiding this comment

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

This is a nitpick, but I find allowedParents to be an odd setting name with a false vaule. It reads like it's trying to communicate "This block is allowed to have a parent block", but I don't find this setting to communicate that immediately or clearly. I went back and read the API in the original comment and this is actually an array of allowed parent types, but the default value of false makes me not expect an array.

Not to 🚲🏠 too much, but what about allowedParentBlockTypes: []?

I think an empty array as the default value lets the code more clearly communicate this setting's expected values. We can check for an empty array length to know no parents are allowed rather than using false to meant "No allowed parent types"

Copy link
Member

Choose a reason for hiding this comment

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

Update: I've seen why this allows true and false types but I think they make the API unclear. I'd rather constants that signified the meaning of what true and false are achieving here instead of just using those values without info.

}

function orderItems( items ) {
return orderBy( items, [ 'utility', 'frecency' ], [ 'desc', 'desc' ] );
Copy link
Member

Choose a reason for hiding this comment

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

Should "frecency" be "frequency"?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not clear on what this does but maybe that's just me being new to the Gutenberg project. It reads like it might be sorting items to appear in the inserter based on some settings in each block, but adding a comment to explain that would be really helpful for anyone who sees this code without a lot of context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed—I wrote about this here but it could definitely use a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment in 0869dec!

@@ -144,7 +147,8 @@ export class InserterMenu extends Component {

sortItems( items ) {
if ( 'suggested' === this.state.tab && ! this.state.filterValue ) {
return items;
const sortedItems = orderBy( items, [ 'utility', 'frecency' ], [ 'desc', 'desc' ] );
Copy link
Member

Choose a reason for hiding this comment

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

Should this be "frequency" too? Or is "frecency" common in the code?

Copy link
Member

Choose a reason for hiding this comment

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

Just did a grep in the code and found it (and the link to https://en.wikipedia.org/wiki/Frecency in a code doc block).

I think you should annotate any instances of "frecency" in the code with a link to that wiki article or something else that lets developers know it's intentional. It looks like "frequency" and could be easily mistaken as a coding error. I search for "frecency" but DuckDuckGo and Google both spell-corrected it too, so it wasn't even obvious to search for clarification on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment in 0869dec!

@@ -144,7 +147,8 @@ export class InserterMenu extends Component {

sortItems( items ) {
if ( 'suggested' === this.state.tab && ! this.state.filterValue ) {
return items;
const sortedItems = orderBy( items, [ 'utility', 'frecency' ], [ 'desc', 'desc' ] );
return sortedItems.slice( 0, MAX_SUGGESTED_ITEMS );
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a test for this code; it would be nice to add a test that ensures the right number of items are displayed. (You could make maxSuggestionItems a prop with a default value of MAX_SUGGESTED_ITEMS too, to avoid adding extra fixtures and make the component a bit more flexible.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test in cd3e91e!

* items (e.g. a regular block type) and dynamic items (e.g. a shared block).
*
* @param {Object} state Global application state.
* @param {string[]|boolean} editorAllowedBlockTypes Allowed block types, or true/false to
Copy link
Member

Choose a reason for hiding this comment

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

Oh, now I see. I find this API confusing and think that using a constant rather than true/false to signify special setting would be good. Having editorAllowedBlockTypes: false and especially editorAllowedBlockTypes: true is hard to understand to me. Something like:

const BLOCK_TYPES_ALL_ALLOWED = 'all';
const BLOCK_TYPES_NONE_ALLOWED = 'none';

[...]

buildInserterItemFromBlockType( state, BLOCK_TYPES_ALL_ALLOWED );

The constants could be shared somewhere, but that reads clearer to me than overloading the true and false values to mean something else for this argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

😅 I agree that using true and false for this is regrettable. Happy to move away from it. We'd have to deprecate() as this API is public.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. Deprecating it would be 👍 😄

* Some examples:
*
* undefined -> { '*': true }
* true -> { '*': true }
Copy link
Member

Choose a reason for hiding this comment

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

What's the benefit to being so lenient with this API? parseAllowList( { '*': true } ) is very clear and obvious to me, but parseAllowList( true ) does not make as clear what the function is parsing or what it will output.

The difference in typing is minimal and we can reduce code complexity by requiring an object as input.

It's especially confusing that, according to the docs, a false-y value (undefined) as an argument should output a wildcard true list (undefined -> { '*': true }), but false does not (false -> { '*': false }).

Copy link
Member Author

@noisysocks noisysocks May 8, 2018

Choose a reason for hiding this comment

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

Supporting true or false is because of backwards compatibility with how allowedBlocks and allowed_block_types work currently. I'd be happy to deprecate() it, though.

undefined has to map to { '*': true } because the default behaviour of e.g. not specifying the allowedBlocks prop is to allow all blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I don’t know why we picked this convention in the first place, but we should deprecate and and use an empty array/object instead of false and something else instead of true.

} );

it( 'should parse a shorthand array', () => {
const allowList = parseAllowList( [ 'core/image', '!core/verse', '*' ] );
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we're doing a lot of work here to avoid using a JS built-in: a plain object with the block name as a key and true/false as the allow value. Is this '!core/verse' type of argument common elsewhere? It seems to be extra code to run and test to avoid writing { 'core/image': false } and instead write [ '!core/image' ]. It doesn't seem worth it to me.

The only benefit is maybe a list of allowed parseAllowList( [ 'core/image', 'core/verse' ] ) => { 'core/image': true, 'core/verse': true } but even then I say we just make objects required for where this is used and save a lot of code and make the API more consistent. Consistent APIs are good and lead to fewer bugs 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

The [ '!core/image' ] syntax was a quick idea that came up while speaking with @gziolo. I'm not attached to it 🙂

I would prefer to keep parseAllowList( [ 'core/image', 'core/verse' ] ) => { 'core/image': true, 'core/verse': true, '*': false } because:

  1. It's intuitive for one to write a simple list of block types that they want to whitelist
  2. Backwards compatibility with how allowedBlocks and allowed_block_types work currently

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that makes sense. I'm cool with keeping just [ 'core/image', 'core/verse' ]; it's a nice shorthand for allowing a list of blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Any ideas how to provide a shorthand for the list of disallowed blocks? 😃

Copy link
Member

@tofumatt tofumatt May 8, 2018

Choose a reason for hiding this comment

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

In terms of an easily understandable API I'd just make it another property. There's less room for error that way (it would be easier to spot what's allowed and what's not)... and actually I don't see why they couldn't be combined behind the scenes. Why not:

// When you want to allow only certain blocks.
<WhateverBlock
  allowedBlocks={ [ 'core/image', 'core/verse' ] }
/>

// When you want to deny only certain blocks.
<WhateverBlock
  disallowedBlocks={ [ 'core/video' ] }
/>

// This should throw an error, because when do you need an allow list
// and a disallow list when you don't set wildcards?
<WhateverBlock
  allowedBlocks={ [ 'core/image', 'core/verse' ] }
  disallowedBlocks={ [ 'core/video' ] }
/>

That makes it way clearer and you don't have to manually set wildcards.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no logical need to allow both allowed and disallowed blocks to be explicitly defined in the list... right? 😄

No, and that's part of why I like the object syntax. When typing { 'core/video': true } you're forced to choose true or false 🙂

Copy link
Member

Choose a reason for hiding this comment

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

The only issue with that is there is wildcard for either true or false after everything else, but the object API allows for explicit allowed and disallowed values. But with an allowed list approach, you don’t need to explicitly disallow any values with a false setting.

The object api allows folks to set things as both true and false which is confusing. It might be better to accept a flat array as either allowed Or disallowed, and then wildcard everything else as the opposite

Copy link
Member Author

@noisysocks noisysocks May 10, 2018

Choose a reason for hiding this comment

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

In a previous version of this PR I tried having only whitelists. The problem with having only whitelists or blacklists, though, is that you can get into situations where a parent and child conflict with one another. For example:

// In foo/parent:
<InnerBlocks allowedBlocks={ [ 'foo/child-1' ] }>

// In foo/child-1:
allowedParents: []

// In foo/child-2:
allowedParents: [ 'foo/parent' ]

Here, it's not clear what should be allowed.

  • Can foo/child-1 be inserted into foo/parent? The parent says yes, but the child says no.
  • Can foo/child-2 be inserted into foo/parent? The parent says no, but the child says yes.

This is what motivated me to introduce the object syntax, which attempts to introduce a distinction between what a block explicitly desires and what it implicitly desires.

// In foo/parent:
<InnerBlocks allowedBlocks={ { 'foo/child-1': true, '*': false } }>

// In foo/child-1:
allowedParents: { '*': false }

// In foo/child-2:
allowedParents: { 'foo/parent': true, '*': false }

Now, things are clearer.

  • Can foo/child-1 be inserted into foo/parent? The parent explicitly says yes, and the child implicily says no. Explicit wins, so the answer is yes.
  • Can foo/child-2 be inserted into foo/parent? The parent implicitly says no, and the child explicitly says yes. Explicit wins, so the answer is yes.

Being able to explicitly say no is useful, too. For example, foo/parent might want to prevent foo/buggy from ever being nested within it because it causes a nasty styling bug.

// In foo/parent:
<InnerBlocks allowedBlocks={ { 'foo/child-1': true, 'foo/buggy': false, '*': false } }>

Here, it doesn't matter what foo/buggy specifies—it won't be allowed in foo/parent.

😅 Sorry about the long comment. I hope that explains some of my thinking behind the object syntax—let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense... but it's very complex and I'd say tough to explain. You need to know whether something was explicitly/implicitly set to know if it should be allowed.

I find this approach unintuitive because I'd imagine different people will intuit different things from the settings.

Why not just err on the side of "any time something is not allowed: answer is no?" It's much simpler and means there isn't a complex formula to whether a block is allowed. If the parent or the child says no: answer is no.

The example:

<InnerBlocks allowedBlocks={ { 'foo/child-1': true, 'foo/buggy': false, '*': false } }>

becomes:

<InnerBlocks allowedBlocks={ [ 'foo/child-1' ] }>

As a new developer coming to this API I'd be confused why there are explicit and wildcard false values.

Copy link
Member Author

@noisysocks noisysocks May 11, 2018

Choose a reason for hiding this comment

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

Why not just err on the side of "any time something is not allowed: answer is no?"

Because if we err on the side of 'no', it means that a plugin developer cannot write a block that is insertable into a core block that uses allowedBlocks (e.g. cover image). This came up in #5540 (comment).

I do concede that the object syntax is difficult to explain. My hope was that the shorthand syntax would help alleviate this—99% of developers could stick to arrays, and then 1% could migrate to the advanced syntax if they encounter one of the above scenarios.

At any rate, I'd like to ship something soon, and so I'd be happy to start with array whitelists (erring on the side of 'yes') and then revisit this later if we receive feedback that suggests that developers need the extra flexibility. Does that sound OK?

return getItemsFromInserts( state, sortedInserts, allowedBlockTypes, maximum );
const items = getInserterItems( state, allowedBlockTypes );
const usefulItems = items.filter( ( item ) => item.utility > 0 );
const sortedItems = orderBy( usefulItems, [ 'utility', 'frecency' ], [ 'desc', 'desc' ] );
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned, I think any reference to 'frecency' should be annotated as it's not an extremely common concept in my experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

This function will be deleted in 3.0 so I won't add a comment here.

if ( 'isPrivate' in settings ) {
deprecated( 'isPrivate', {
version: '3.0',
alternative: 'allowedPostTypes',
Copy link
Contributor

Choose a reason for hiding this comment

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

isPrivate means the block is available programmatically but not in the UI. When I read allowedPostTypes or allowedParents, it looks like in these case the blocks shouldn't be available entirely (even programmatically). So for me it seems that these two should be separate options?

The difference is semantic for me, which means if I use isPrivate the block can be inserter everywhere without issue. While we could imagine adding a warning or an error if the block is used at a position that goes against its allowedParents config.

Copy link
Member Author

Choose a reason for hiding this comment

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

isPrivate: true and allowedParents: [] behave the same so my thought was to consolidate them. You're right that there's a slight semantic difference, though. I'm happy to add isPrivate back 🙂

cc. @gziolo

Copy link
Member

@gziolo gziolo May 8, 2018

Choose a reason for hiding this comment

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

I was thinking we could convert isPrivate to become one of the blocks’s supports property with a name like insertable defaulting to true.
I thought that consolidation is also fine when I saw this proposal. I’m fine with both. Having it as individual property is not that clear in the current form for plugin developers.

Copy link
Member Author

Choose a reason for hiding this comment

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

supports.inserting kind of makes sense to me. I'm happy with that.

if ( ! allowedBlockTypes || ! sharedBlock ) {
return null;
}
const calculateUtility = ( category, count, isContextual ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand properly the utility is essentially usefull when displaying the "suggested" category right?

Order the items in the inserter: Suggested > frecent > common

And it's also used to exclude items from the Suggestions (utility = 0).

I think it's a bit confusing (maybe just because it's a new API), what if we don't filter at all, if we splice later (seems like we do), we don't need to filter.

Copy link
Member Author

@noisysocks noisysocks May 8, 2018

Choose a reason for hiding this comment

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

Yep, the behaviour of the Suggested tab is now to show items in the following order:

  1. Blocks that are contextually useful. For example, if you are inserting into a Product block, Add to Cart is contextually useful.
  2. Blocks that have been inserted by the current user in the past, ordered by how recently/frequently they were used.
  3. Blocks that are in the common category.
  4. All other blocks.

I introduced utility as a way to achieve this, since the inserter logic was growing quite complex, e.g. fillWithCommonBlocks and getSuggestedInserterItems were becoming difficult to reason about. By having utility and frecency in the item object, we can instead simply ORDER BY utility DESC, frecency DESC.

* ordering items by relevancy.
*/
export const getInserterItems = createSelector(
( state, editorAllowedBlockTypes, parentUID = null ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully with #6500 we'd be able to remove the second arg entirely.

* Some examples:
*
* { 'core/image': true, '*': false } specifies that only images are allowed.
* { 'core/verse': false, '*': true } specifies that everything except verses are allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to support the block lists? I'm asking cause it seems redundant with the allowedBlocks prop of the InnerBlock component. and in that case we could simplify the API here and just provider a list of allowed parents.

Copy link
Member Author

@noisysocks noisysocks May 7, 2018

Choose a reason for hiding this comment

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

Do you mean allow the advanced syntax ({ 'foo': false, 'bar': true, '*': true }) in allowedBlocks but only simple arrays ([ 'foo', 'bar' ]) in allowedParents?

My preference is that we support the same syntax in allowed_block_types (the PHP filter), allowedBlocks (the InnerBlocks prop), and allowedParents since it's no extra work for us and introduces some consistency throughout the entire blocks API.

cc. @gziolo

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, what I meant is that we should only support [ 'foo', 'bar' ] in all of these, as it's simple and I believe we can achieve everything with it? unless I'm missing a use-case?

Copy link
Member

Choose a reason for hiding this comment

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

Doing exceptions is harder with the array-only list (how do you make ({ 'foo': false, 'bar': true, '*': true })?), so unless we keep the ['!foo', 'bar', '*'] option around (which is more code/complexity and less understandable as plain JS) we'd need to allow objects as arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to do exceptions? What use-cases can't be achieved with a combination of allowParents and allowedBlocks (which is allowChildren)?

Copy link
Member

Choose a reason for hiding this comment

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

Actually we need to cover two cases which are:

  • list all allowed blocks
  • list all disallowed blocks, which is in other words, all but listed blocks

Copy link
Member Author

@noisysocks noisysocks May 9, 2018

Choose a reason for hiding this comment

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

What use-cases can't be achieved with a combination of allowParents and allowedBlocks (which is allowChildren)?

A plugin developer wouldn't be able to e.g. create a block that allows all children except for core/image because they can't change the allowedParents of an already registered block.

Do we really need to do exceptions?

I mean—we don't really need to do anything 😛 But my thinking was that it could be useful for plugin developers.

In the interest of arriving at a consensus I'd be happy to start with only whitelist arrays. We can expand to allowing whitelist+blacklist objects in the future based on feedback we get from plugin developers.

Let me know what y'all think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you convinced me we need both, particularly this argument 👍

A plugin developer wouldn't be able to e.g. create a block that allows all children except for core/image because they can't change the allowedParents of an already registered block.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I personally don't like the ambiguity between post types and blocks :) what if I want a block to be allowed in a parent block plugin/product but not the CPT plugin/product. Granted that it's not too common but I still prefer explicitness.

@noisysocks
Copy link
Member Author

noisysocks commented May 8, 2018

Thanks for the thoughts @tofumatt and @youknowriad!

Beyond that, I think the list arguments are too fancy for little benefit. It means various blocks will use a slightly different API for declaring Allowed Parent Block Types making it hard to easily work on different blocks depending on who coded it and which API they prefer. I think it could use simple JS objects with '': true and '': false` as special values, rather than the much more flexible and less consistent argument types it takes now.

You're right that it's probably overkill. I'm thinking let's only support arrays and objects, e.g.:

parseAllowList( [] ) => { '*': false }
parseAllowList( [ 'core/image' ] ) => { 'core/image': true, '*': false }
parseAllowList( [ 'core/image': false, '*': true ] ) => { 'core/image': false, '*': true }

I think this is a good balance between plugin developer ergonomics and Gutenberg complexity.

Booleans would also be supported for a short while with a console warning that tells folks to replace allowedBlocks={ false } with allowedBlocks={ [] } and allowedBlocks={ true } with nothing.

I personally don't like the ambiguity between post types and blocks :) what if I want a block to be allowed in a parent block plugin/product but not the CPT plugin/product. Granted that it's not too common but I still prefer explicitness.

I think @mtias was the only one pushing for this. I'm happy to go either way on it, with a slight preference for having less API surface area since it's rare that a CPT contains a /.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Definitely a fan of the simpler, consolidated API for allowedBlocks={} 👍

@gziolo
Copy link
Member

gziolo commented May 8, 2018

I’m all for discontinuing support for boolean values. We also need to keep im mind that we need to pick a schema which can be expressed in both PHP and JS, because we use this filter on the server which allows to pick which blocks can be inserted. At the moment we support only whitelist approach on the server, but theres is expectation to provide a way to support all blocks but those listed. I personally would seek for solution which is flexible enough to prevent us from introducing another property.

@noisysocks
Copy link
Member Author

noisysocks commented May 9, 2018

Quick update on this, since there's a few disparate conversations going on in threads:

I'm aiming to get a consensus on the API before I continue any further. We're also waiting on some thoughts from @mtias who said he'll pop in soon.

I think we all agree on:

  • Deprecating booleans e.g. allowedBlocks={ false } and add_filter( 'allowed_block_types', function() { return false; } ) because it's confusing and specifying e.g. an empty array is more meaningful
  • Any syntax we choose has to work in both PHP and JavaScript
  • Rename isPrivate to something like supports.insertion instead of deprecating it
  • Support only whitelist arrays in allowedBlocks and allowedParents. We can add blacklisting later if we receive feedback that suggests that developers need the extra flexibility

Still contentious:

  • Should allowedParents be used for post types as well as block types? e.g. allowedParents: [ 'post', 'page', 'plugin/cool-block' ] Or should a separate property be introduced? e.g. allowedParentBlocks: [ 'plugin/cool-block' ], allowedPostTypes: [ 'post', 'page' ]

EDIT (2018-05-11): Updated to reflect decision made in #6346 (comment)

@mtias
Copy link
Member

mtias commented May 14, 2018

Let's punt the decision on supporting post types under the same API so that it doesn't block us from merging this.

@noisysocks
Copy link
Member Author

Thanks for the thoughts, y'all. Going to close this and split something small and mergeable from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants