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

Avoid confusion around blockTypes/allowedBlockTypes/enabledBlockTypes setting #6069

Merged
merged 3 commits into from
Apr 10, 2018

Conversation

noisysocks
Copy link
Member

Quick cleanup that does two things.

First, it makes the enabledBlockTypes argument in getInserterItems and getFrecentInserterItems non-optional. I regret making this optional since it implies that not providing this argument is a sensible thing to do, when in reality it would cause subtle bugs, e.g. a user could insert a block that has been disabled via the allowed_block_types filter.

Since selectors are exposed publicly now, I added a deprecation warning so as to avoid breaking backwards compatibility. Let me know if you think that's overkill.

Secondly, I noticed that we refer to this array of allowed block types throughout the codebase as either blockTypes, allowedBlockTypes or enabledBlockTypes. I've renamed these variables so that we are always referring to this array as allowedBlockTypes, which matches the name of the filter.

How I tested this

  1. Create a post, insert some regular and shared blocks. There should be no warnings in the console
  2. Add the following to the top of lib/load.php:
add_filter( 'allowed_block_types', function() {
	return [ 'core/paragraph', 'core/image', 'core/quote' ];
} );
  1. Create a post. Test that only paragraphs, images and quotes can be inserted via the inserter and quick inserter (the thing that appears to the right of Write your story).

Having this argument optional implies that it's safe to not provide one,
when in reality that will probably result in a bug.
Using the same name for this feature throughout the codebase will
hopefully help developers search for it and better understand it.
@noisysocks noisysocks added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Apr 9, 2018
@noisysocks
Copy link
Member Author

cc. @brandonpayton

@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Apr 9, 2018
return null;
}

const blockTypeIsDisabled = Array.isArray( enabledBlockTypes ) && ! includes( enabledBlockTypes, 'core/block' );
const blockTypeIsDisabled = Array.isArray( allowedBlockTypes ) && ! includes( allowedBlockTypes, 'core/block' );
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR, but I wanted to double check if we need to open another issue. Shouldn't this logic verify whether core/block isn't allowed, but also referencedBlockType.name?

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion, disabling core/block should prevent you from inserting all shared blocks, but I could be persuaded otherwise. Let's discuss in #5893.


return {
hasSupportedBlocks: true === blockTypes || ! isEmpty( blockTypes ),
hasSupportedBlocks: true === allowedBlockTypes || ! isEmpty( allowedBlockTypes ),
Copy link
Member

Choose a reason for hiding this comment

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

Since this line is updated. It can be simplified to:

hasSupportedBlocks: ! isEmpty( allowedBlockTypes )

Copy link
Member Author

@noisysocks noisysocks Apr 10, 2018

Choose a reason for hiding this comment

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

Very confusingly for us PHP folks, _.isEmpty() doesn't work the same way as empty():

$ node
> var _ = require('lodash');
> _.isEmpty(true)
true
> _.isEmpty(false)
true

@gziolo
Copy link
Member

gziolo commented Apr 9, 2018

Sharing for a reference from https://wordpress.org/gutenberg/handbook/extensibility/extending-blocks/#hiding-blocks-from-the-inserter:

Hiding blocks from the inserter

On the server, you can filter the list of blocks shown in the inserter using the allowed_block_types filter. you can return either true (all block types supported), false (no block types supported), or an array of block type names to allow.

I like the changes introduced in this PR 👍

export function getInserterItems( state, allowedBlockTypes ) {
if ( allowedBlockTypes === undefined ) {
allowedBlockTypes = true;
deprecated( 'getInserterItems with no allowedBlockTypes argument', {
Copy link
Member

Choose a reason for hiding this comment

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

Both depreciation messages should be also added to https://github.com/WordPress/gutenberg/blob/master/docs/deprecated.md for better visibility.

@noisysocks noisysocks merged commit 04cbab4 into master Apr 10, 2018
@noisysocks noisysocks deleted the fix/require-getInserterItems-argument branch April 10, 2018 03:19
@noisysocks noisysocks added this to the 2.7 milestone Apr 13, 2018
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 [Feature] Inserter The main way to insert blocks using the + button in the editing interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants