Block settings extensibility #7895

oandregal · 2018-07-11T16:07:44Z

Add a Slot in the BlockSettingsMenu for third-parties to hook into.

onClick will be called when the user clicks the menu item.
allowedBlocks should be a whitelist of block names. If not present the item will be shown for every block. If multiple blocks are selected, the item will be shown if all of them are in the whitelist.

An example on how a plugin would use this API:

import { registerPlugin } from '@wordpress/plugins';
import { PluginBlockSettingsMenuItem } from '@wordpress/editPost';

const doOnClick = ( ) => {
    // the plugin doing its dance
};

registerPlugin( 'plugin-name', {
    render: ( ) => (
        <PluginBlockSettingsMenuItem 
            allowedBlockNames=[ 'core/paragraph' ]
            icon='dashicon-name'
            label='Plugin name'
            onClick={ doOnClick } />
    ),
} );

Open questions

should the API allow whitelist/blacklist the block types in which this item should be shown? If so, what would be the best way to declare this? I was thinking about adding a new prop blockList to the BlockSettingsMenuPluginsItem that would contain the block names. Mimic the allowedBlocks property of InnerBlocks.
~~is the block UID enough info for the callback or do we anticipate the plugin would need anything else?~~ Plugin authors should use the selectors in core data package to get access to the selected blocks.

Testing

Download and activate the test plugin.
Make sure a new item in the settings menu is shown for paragraph blocks, and the icon is screenoptions. On clicking, Block clicked is logged to the console.
Modify the allowedBlocks property (no prop, different block names, multiple selections, etc) and test that results are expected.

oandregal · 2018-07-12T15:23:53Z

should the API allow whitelist/blacklist the block types in which this item should be shown?

See allowedBlocks property in InnerBlocks. We probably want to mimic that.

oandregal · 2018-07-12T16:12:18Z

From https://wordpress.slack.com/archives/C02QB2JS7/p1531410854000014 instead of passing the block uid to the callback, we may want to encourage plugin authors to use the selectors in the core data package.

jorgefilipecosta

I think this is looking good 👍
One thing that may worths discussion is if we should pass the allowedBlocks prop to the exposed slot/fill component and have a simple BlockSettingsItem for UI instead of doing the fill directly in the item component.

jorgefilipecosta · 2018-07-13T20:08:07Z

editor/components/block-settings-menu/block-settings-menu-plugins-group.js

+);
+
+BlockSettingsMenuPluginsGroup.Slot = withSelect( ( select, { fillProps: { uids } } ) => ( {
+	selectedBlocks: map( flatten( [ uids ] ), ( uid ) => select( 'core/editor' ).getBlockName( uid ) ),


map will generate a new array reference every time the store changes which may cause unnecessary rerenders. Maybe we can use elector getBlocksByUID which is cached and just that array down.

jorgefilipecosta · 2018-07-13T20:22:24Z

editor/components/block-settings-menu/block-settings-menu-plugins-item.js

+import BlockSettingsMenuPluginsGroup from './block-settings-menu-plugins-group';
+
+const shouldRenderItem = ( selectedBlockNames, allowedBlockNames ) => ! Array.isArray( allowedBlockNames ) ||
+	difference( selectedBlockNames, allowedBlockNames ).length === 0;


difference( selectedBlockNames, allowedBlockNames ).length === 0; seems like a simple way to check if all all items of selectedBlockNames are contained in allowedBlockNames.
But it makes people reading the code having to think a little bit about what the code is doing. Maybe we can have a simple function arrayContainsArray or something similar and use it in shouldRenderItem plus a comment saying that item is rendered if allowedBlocks is not set or if all selectedBlocks are included in allowedBlocks.

jorgefilipecosta · 2018-07-13T20:39:03Z

editor/components/block-settings-menu/block-settings-menu-plugins-item.js

+const shouldRenderItem = ( selectedBlockNames, allowedBlockNames ) => ! Array.isArray( allowedBlockNames ) ||
+	difference( selectedBlockNames, allowedBlockNames ).length === 0;
+
+const BlockSettingsMenuPluginsItem = ( { allowedBlocks, icon, label, onClick, small, role } ) => (


Plugins may want to have multiple menu items and even have their own separators to separate their items. In that case, they would need to be repeating the allowedBlocks prop on each item plus use the BlockSettingsMenuPluginsGroup to pass the separator.
Maybe we can consider exposing the slot in a way that does not render anything and receives the allowed block props. And exposes a simple component called SettingMenuItem that just uses IconButton and handles the mobile label logic.
E.g:

<PluginBlockSettingsMenu allowedBlocks={ [...] }> <BlockSettingMenuItem icon={} label="Item of the first group" onClick={} /> ... <div className="editor-block-settings-menu__separator" /> <BlockSettingMenuItem icon={} label="Item of the second group" onClick={} /> ... </PluginBlockSettingsMenu>

This would also allow plugins more options to customize their UI.

I like the idea of enabling plugins to introduce more than one item on the block settings menu.

How about this API?

<BlockSettingsMenuPluginsItem allowedBlockNames=[ 'core/paragraph' ] secondaryMenu=[ { label: 'Secondary 1', onClick: { secondary1OnClick } }, { label: 'Secondary 2', onClick: { secondary2OnClick } }, ], icon='dashicon-name' label='Plugin name' onClick={ primaryOnClick } />

The idea would be to enable plugins to add either one or multiple items, in an browsers-like fashion (compare LastPass VS Save to Pocket items):

If BlockSettingsMenuPluginsItem receives a secondaryMenu property it'll introduce a secondary menu and an arrow in the item to communicate there is another level. If secondaryMenu is not present it'll render a normal item. On mobile, we may want to use a different pattern (for ex: render the two items consecutively in the primary menu).

This doesn't address allowing plugins to add separators. I lean towards thinking separators should be UI only core can add - by staying the same no matter the plugins an user has installed Gutenberg communicates clearly how the menu is organized.

What do you think?

Hi @nosolosw, I like the idea of submenus so plugins can group their actions if they have multiple operations.
This proposal introduces a new type of menus that we don't have in Gutenberg yet cc: @karmatosed .

Regarding the technical point of view:
What if we have a component that allows the rendering of multiple items like:

<MultipleItemsMenu icon='dashicon-name' label='Plugin name'" > <MenuItem label="Secondary 1" onClick={} /> <MenuItem label="Secondary 2" onClick={} /> </MultipleItemsMenu>

It would be a simple UI component that even core could use.

If we allow plugins to render what they want to the slot, then they could use this component as:

<PluginBlockSettingsMenu allowedBlocks={ [...] }> <MultipleItemsMenu icon='dashicon-name' label='Plugin name'" > ... </PluginBlockSettingsMenu>

Both approaches are similar both if we use react components we have some things simplified.
Using secondaryMenu=[ { label: 'Secondary 1', onClick: { secondary1OnClicl } ..., Will create a new array reference and new object references per item inside the array. For plugins to avoid rerenders they will need to have some logic to cache the references and only update the references when onClick or labels change. If we represent the same info as React components that logic is not needed.

As per private conversations with Tammie, she suggested that plugins shouldn't be able to have the flexibility to customize the slot at this point (adding separators, etc).

There are a couple of extra considerations we haven't considered yet and we should take into account:

the API surface of this should be similar to the one of PluginSidebarMenuItem, as to create a coherent experience for developers.

plugins may want to have a different set of allowedBlocks per item.

All things considered, I think we should go with the API as it is in the PR, as it meets all requirements (plugins can have more than one item, items can have different allowedBlocks sets, developers can't add their own separators/components, and the API shape is the same than the PlugiSidebarMenuItem). I still think a component for secondary menus could be useful, but that can be added later if it proves necessary/useful.

Thanks for your time, Jorge! This conversation has helped me to understand better the Gutenberg APIs.

jorgefilipecosta · 2018-07-13T20:40:49Z

editor/components/block-settings-menu/block-settings-menu-plugins-group.js

+	selectedBlocks: map( flatten( [ uids ] ), ( uid ) => select( 'core/editor' ).getBlockName( uid ) ),
+} ) )( BlockSettingsMenuPluginsGroupSlot );
+
+export default BlockSettingsMenuPluginsGroup;


Normally we start the extensibility slot names with Plugin e.g: PluginPostPublishPanel, PluginPostStatusInfo etc... So it may make sense to call this component something like PluginBlockSettingsMenu.

As per #7895 (comment) reverted this change: the external API lives now in edit-post/block-settings-menu and can be used as PluginBlockSettingsMenuItem.

jorgefilipecosta · 2018-07-13T20:52:28Z

editor/components/block-settings-menu/block-settings-menu-plugins-item.js

+			}
+			return ( <IconButton
+				className="editor-block-settings-menu__control"
+				onClick={ compose( onClick, onClose ) }


compose will call: onClick( onClose( ...arg ) ); I think what we want here is:

( ...args ) => { onClick( ...args ); onClose(); }

Note that the onClick API isn't passed any props, so

onClick( onClose() );

VS

( ) => { onClick( ); onClose(); }

is essentially the same (omiting the sequence they're called, which doesn't matter as closing and executing the action are parallel processes).

I thought we passed arguments to onClick, if we don't pass them it is exactly the same.

gziolo · 2018-07-16T12:03:15Z

editor/components/index.js

@@ -83,6 +83,8 @@ export { default as BlockMover } from './block-mover';
 export { default as BlockSelectionClearer } from './block-selection-clearer';
 export { default as BlockSettingsMenu } from './block-settings-menu';
 export { default as _BlockSettingsMenuFirstItem } from './block-settings-menu/block-settings-menu-first-item';
+export { default as PluginBlockSettingsMenuGroup } from './block-settings-menu/plugin-block-settings-menu-group';


All plugin specific code should live outside of editor folder. This is purely design decision. The idea is that editor is meant to provide all the pieces to required to build your own editor interface. edit-post is one of the examples of this implementation. You can check how this was done for publish panels in here:
https://github.com/WordPress/gutenberg/blob/master/editor/components/post-publish-panel/index.js#L88-L99

Related PR and discussion: #6798.

gziolo · 2018-07-16T12:06:12Z

editor/components/block-settings-menu/index.js

@@ -99,6 +100,7 @@ export class BlockSettingsMenu extends Component {
 							{ count === 1 && <BlockHTMLConvertButton uid={ firstBlockUID } role="menuitem" /> }
 							<BlockDuplicateButton uids={ uids } rootUID={ rootUID } role="menuitem" />
 							{ count === 1 && <SharedBlockConvertButton uid={ firstBlockUID } onToggle={ onClose } itemsRole="menuitem" /> }
+							<PluginBlockSettingsMenuGroup.Slot fillProps={ { uids, onClose } } />


See also related discussion related to using slots in here:
#7199 (comment)

There are a few important point discussed and you can also learn why:

<_BlockSettingsMenuFirstItem.Slot fillProps={ { onClose } } />

exists in the first place and why it is considered as internal implementation.

01b045b takes this and #7895 (comment) into account and exposes the API as PluginBlockSettingsMenuItem.

gziolo · 2018-07-19T09:51:50Z

edit-post/components/block-settings-menu/index.js

@@ -0,0 +1,4 @@
+import { _BlockSettingsMenuPluginsItem } from '@wordpress/editor';
+
+const PluginBlockSettingsMenuItem = _BlockSettingsMenuPluginsItem;


PluginPrePublishPanel which I referenced as an example is fully defined inside edit-post module, and passed down to the component imported from editor module as a prop. Is there any reason we shouldn't be doing the same in here?

I think I've addressed this concern with the latest changes, editor and edit-post are decoupled now.

gziolo · 2018-07-19T09:58:09Z

editor/components/block-settings-menu/block-settings-menu-plugins-group.js

+		<Slot fillProps={ { ...fillProps, selectedBlocks } } >
+			{ ( fills ) => ! isEmpty( fills ) && (
+				<Fragment>
+					<div className="editor-block-settings-menu__separator" />


Wouldn't it be more flexible if produce the following code:

<div className="class-name..."> { fills } </div>

In More Menu we don't use explicit DOM elements as separators, we rather use CSS styles to achieve a similar result. In this scenario, the placement of separator is fully dependent on the sibiling componets which is not optimal.

Note that this is how the BlockSettingsMenu works at the moment, which is different from the MoreMenu. I'd like to leave this out of this PR as it's a different concern.

oandregal · 2018-07-26T10:48:10Z

@jorgefilipecosta @gziolo I think I've addressed all feedback provided. Can this be merged?

oandregal · 2018-07-31T09:40:17Z

This has been rebased from master (there was some conflicts because editor had been moved to its own package).

jorgefilipecosta

Hi @nosolosw, things worked correctly in my tests and the code looks good to me.

There is a design detail that I noticed. When we have just one plugin I think we may be missing a separator between plugins and remove:

If we add two or more plugins the separator appears:

We should also add some docs to this API not certain if here or in a followup PR.

gziolo · 2018-08-02T05:57:59Z

We should also add some docs to this API not certain if here or in a followup PR.

Docs are essential for any public API. They should be added to the matching section in https://github.com/WordPress/gutenberg/blob/master/docs/extensibility/extending-blocks.md.

oandregal · 2018-08-02T08:16:19Z

Added docs in d51b5ee.

@jorgefilipecosta Tried with Chrome and Firefox (Ubuntu OS) but I can't repro the issue. I can't figure out why that would happen: that separator element is static, not dependant on any other item. Would it be too much to ask to clean up the branch and try again?

jorgefilipecosta · 2018-08-02T23:36:49Z

I'm sorry for the trouble cause @nosolosw it looks like something was wrong in my browser state in a private window and in other browsers the design was right.

jorgefilipecosta

The code looks good to me and it tested great 👍 Thank you for iterating on this @nosolosw!

oandregal · 2018-08-03T07:57:53Z

I can't thank you enough for helping me through this review @jorgefilipecosta and @gziolo My Gutenknowledge grew by an order of magnitude! ❤️

editor exposes the _BlockSettingsMenuPluginsExtension component that specific implementations can use. edit-post chooses to fill that slot with a component of their own (PluginBlockSettingsMenuGroup) which happens to be a slot as well. This way, third-party authors can hook into the edit-post specific implementation by using the PluginsBlockSettingsMenuItem fill.

aduth · 2018-08-07T19:07:02Z

edit-post/components/block-settings-menu/plugin-block-settings-menu-group.js

+};
+
+PluginBlockSettingsMenuGroup.Slot = withSelect( ( select, { fillProps: { clientIds } } ) => ( {
+	selectedBlocks: select( 'core/editor' ).getBlocksByUID( clientIds ),


getBlocksByUID was deprecated in Gutenberg v3.3 and is slated to be removed in the next release. A warning would have been logged to your console on its use.

aduth · 2018-08-07T19:43:42Z

edit-post/components/block-settings-menu/plugin-block-settings-menu-item.js

+ * WordPress dependencies
+ */
+import { IconButton } from '@wordpress/components';
+import { compose } from '@wordpress/element';


Similarly, wp.element.compose was deprecated in v3.3 and is slated to be removed in the next release. See #7948

aduth · 2018-08-07T19:45:29Z

Worth pointing out that the removal of the deprecated features caused no test failures, meaning test coverage is non-existent for this functionality, and was only caught by pure luck in project search.

oandregal · 2018-08-08T09:11:15Z

@aduth Good catch.

I haven't noticed the warnings, otherwise, I'd have addressed them. Now that I'm aware that's logged, will look into that thoroughly next time, thanks.

aduth · 2018-08-08T15:09:45Z

In fairness, it's not entirely on you that a deprecation was overlooked, as it's a requirement of the deprecation process (and common point of conversation) that it be communicated effectively. Console logging is decent, but obviously not the best solution. It would be nice if we could surface this through the UI, plugin update interface, and/or automated tests (I've been considering introducing console overrides in E2E tests to observe anything logged to console while tests are run).

aduth · 2018-08-08T16:08:39Z

I've been considering introducing console overrides in E2E tests to observe anything logged to console while tests are run

See #8721

phpbits · 2019-03-22T11:07:31Z

@aduth @jorgefilipecosta Seems to be working but with different styling from the rest. Am I missing something? Thanks!

<PluginBlockSettingsMenuItem icon='visibility' label={ __( 'Visibility Settings' ) } onClick={ () => { //onClick action here } } >

oandregal · 2019-03-22T11:50:13Z

Thanks for reporting @phpbits I created #14567 to track this. I'll look into it.

oandregal · 2019-03-22T12:10:52Z

@phpbits #14569 fixes the issue.

phpbits · 2019-03-22T12:17:16Z

@nosolosw Wow! That's fast :) I'll wait for the merge. Thanks!

oandregal self-assigned this Jul 11, 2018

oandregal added [Feature] Extensibility The ability to extend blocks or the editing experience [Status] In Progress Tracking issues with work in progress labels Jul 11, 2018

oandregal force-pushed the update/extend-block-settings branch from d86bbad to 4c25f62 Compare July 12, 2018 09:54

oandregal removed the [Status] In Progress Tracking issues with work in progress label Jul 12, 2018

oandregal requested review from gziolo and aduth July 12, 2018 14:58

oandregal added the [Status] In Progress Tracking issues with work in progress label Jul 12, 2018

oandregal requested a review from jorgefilipecosta July 13, 2018 15:10

oandregal removed the [Status] In Progress Tracking issues with work in progress label Jul 13, 2018

jorgefilipecosta reviewed Jul 13, 2018

View reviewed changes

oandregal force-pushed the update/extend-block-settings branch from 303efce to 1237f0b Compare July 16, 2018 11:52

gziolo reviewed Jul 16, 2018

View reviewed changes

gziolo reviewed Jul 19, 2018

View reviewed changes

oandregal force-pushed the update/extend-block-settings branch 2 times, most recently from 9cec012 to 566a1b8 Compare July 26, 2018 10:19

oandregal force-pushed the update/extend-block-settings branch from 566a1b8 to abd5385 Compare July 31, 2018 09:39

jorgefilipecosta reviewed Aug 1, 2018

View reviewed changes

oandregal force-pushed the update/extend-block-settings branch from abd5385 to d51b5ee Compare August 2, 2018 08:12

jorgefilipecosta approved these changes Aug 2, 2018

View reviewed changes

gziolo mentioned this pull request Aug 3, 2018

Editor: Stop exposing internal components starting with underscore #8421

Closed

oandregal added 6 commits August 7, 2018 11:48

Allow plugins to extend the BlockSettingsMenu

59c9b19

Change uids to clientIds

360dc9d

Move components to edit-post

3d216ad

Adapt to rebase

0e6d2f8

Add docs

740f68e

oandregal force-pushed the update/extend-block-settings branch from d51b5ee to 740f68e Compare August 7, 2018 09:57

oandregal merged commit 8fdf226 into master Aug 7, 2018

oandregal deleted the update/extend-block-settings branch August 7, 2018 10:09

aduth reviewed Aug 7, 2018

View reviewed changes

aduth mentioned this pull request Aug 7, 2018

Framework: Remove deprecations slated for 3.5 removal #8687

Merged

oandregal mentioned this pull request Mar 22, 2019

Block settings item added by plugin not aligned #14567

Closed

swissspidy mentioned this pull request Apr 1, 2019

Fix property name in PluginBlockSettingsMenuItem #14741

Merged

5 tasks

dependabot-preview bot mentioned this pull request Dec 31, 2019

Bump @wordpress/i18n from 2.0.0 to 3.7.0 wearerequired/rest-likes#97

Closed

dependabot-preview bot mentioned this pull request Feb 4, 2020

Bump @wordpress/jest-console from 3.4.0 to 3.5.0 tdreyno/druyan#215

Merged

dependabot-preview bot mentioned this pull request Apr 15, 2020

Bump @wordpress/babel-plugin-makepot from 3.4.0 to 3.5.0 swissspidy/gutenberg-i18n-block#93

Merged

		@@ -0,0 +1,4 @@
		import { _BlockSettingsMenuPluginsItem } from '@wordpress/editor';

		const PluginBlockSettingsMenuItem = _BlockSettingsMenuPluginsItem;

Block settings extensibility #7895

Block settings extensibility #7895

Conversation

oandregal commented Jul 11, 2018 • edited Loading

Open questions

Testing

oandregal commented Jul 12, 2018

oandregal commented Jul 12, 2018

jorgefilipecosta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

oandregal Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

oandregal Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal commented Jul 26, 2018

oandregal commented Jul 31, 2018

jorgefilipecosta left a comment • edited Loading

Choose a reason for hiding this comment

gziolo commented Aug 2, 2018

oandregal commented Aug 2, 2018

jorgefilipecosta commented Aug 2, 2018

jorgefilipecosta left a comment

Choose a reason for hiding this comment

oandregal commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Aug 7, 2018

oandregal commented Aug 8, 2018

aduth commented Aug 8, 2018 • edited Loading

aduth commented Aug 8, 2018

phpbits commented Mar 22, 2019 • edited Loading

oandregal commented Mar 22, 2019

oandregal commented Mar 22, 2019

phpbits commented Mar 22, 2019

oandregal commented Jul 11, 2018 •

edited

Loading

jorgefilipecosta left a comment •

edited

Loading

jorgefilipecosta Jul 13, 2018 •

edited

Loading

jorgefilipecosta Jul 13, 2018 •

edited

Loading

oandregal Jul 16, 2018 •

edited

Loading

jorgefilipecosta Jul 17, 2018 •

edited

Loading

oandregal Jul 20, 2018 •

edited

Loading

jorgefilipecosta Jul 13, 2018 •

edited

Loading

oandregal Jul 16, 2018 •

edited

Loading

jorgefilipecosta left a comment •

edited

Loading

aduth commented Aug 8, 2018 •

edited

Loading

phpbits commented Mar 22, 2019 •

edited

Loading