Add More Menu Item Api

[abotteram]

Props to @mfolkeseth, he did the initial set up of this PR.

Description

Adds an API to add a menu item for plugins to the ellipsis menu.

Using this PR in regards to the registerSidebar API:

wp.editPost.registerSidebar( 
    'plugin-namespace/plugin-id', 
    { title: "Plugin Title", render: () => "Plugin content" }
);

wp.editPost.registerEditorMenuItem( 'plugin-namespace/plugin-menu-item', {
	'title': "Name",
	'target': 'plugin-namespace/plugin-id',
	'icon': () => {
		// Return SVG as react component.
		return (
			<svg>
				<circle cx={50} cy={50} r={10} fill="red" />
			</svg>
		)
	}
} );

How Has This Been Tested?

It has been tested with 1, 0 and more added menu items.
I used console.log's to see whether the callbacks are actually called.

Screenshots (jpeg or gifs if applicable):

Types of changes

New feature.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows has proper inline documentation.

[abotteram]

I would like to voice some concerns here. Since we have chosen to decouple the ellipsis menu item API from the sidebar, a few issues have come up while implementing this:

• What about pinning plugins to the header bar? Will this be a separate API?
• How to determine whether a plugin is "active" in the ellipsis menu (by showing a check icon next to the plugin name)? Since the action that is triggered by clicking the menu item is nothing more than a callback.

[jasmussen]

Thanks for working on this.

What about pinning plugins to the header bar? Will this be a separate API?

What kind of plugins?

Right now we are looking at editor plugins being able to:

• Add sidebars
• Add popouts
• Add separate screen takeovers

At least 1 and 2 should be able to have a common UI for pinning themselves to the editor bar, by a user tapping the "Star" button.

CC: @gziolo in case I said something wrong ;)

How to determine whether a plugin is "active" in the ellipsis menu (by showing a check icon next to the plugin name)? Since the action that is triggered by clicking the menu item is nothing more than a callback.

Here's how it works according to the current plan.

1: A plugin adds an ellipsis menu item

2: Picking this menu item toggles the plugin action

In this case, picking WolframAlpha toggles the WolframAlpha sidebar:

That's all this menu item ever does.

3: If the user presses the "Star" button, the plugin gets pinned to the editor bar

4. When a plugin is pinned to the editor bar, the icon stays there even if you untoggle the sidebar

Even though WolframAlpha is now pinned to the editor bar, the ellipsis menu item stays there. And it does the same thing it always did: it toggles the sidebar.

Meta note: we may find that we need to tweak initial plans as we work on this stuff and discover edge cases or weirdnesses. Just because we have a current plan/idea for how this should work, doesn't mean it's set in stone. But it's also good to iterate in small PRs, instead of trying to tackle the whole structure at once.

[mcsf]

Thanks for working on this. This first review focuses on the code itself and not the general solution to the extensibility problem. I'll gather some thoughts on that and circle back.

[mcsf]

Prefer import { isFunction } from 'lodash';

[mcsf]

Here and in other section comments, format should be

/**
 * External dependencies
 */

[mcsf]

Should be "its own divider".

[mcsf]

Trailing space at end of line.

[mcsf]

Passing the identity function to connect would've been an antipattern. :) Luckily, state doesn't seem to used at all in Plugins, so this enhancement should be removed.

[mcsf]

Worth noting that, by sourcing available items this way, Plugins won't re-render if a menu item is registered dynamically after init.

[abotteram]

The question is whether you want the ellipsis menu to be updated during runtime? This could lead to some bad user experience.

[mcsf]

I would've pointed out that this expr is equivalent to pick( ellipsisMenuItems, [ 'value', 'label', 'icon' ] ), but I don't think we need to construct this at all: since we never pass plugins (we only extract its properties into IconButton), we don't need to limit exposure. In other words, it's fine to drop const plugins and read directly from ellipsisMenuItems in the render map.

[mcsf]

Consider destructuring props in function signature:

function Plugins( { instanceId } ) {

[mcsf]

Formatting for multi-line elements should be:

<div
  key="…"
  className="…"
/>,

[mcsf]

Might be a good time to make a proper MenuItemsSeparator or at least EllipsisMenuSeparator and refactor the divs in editor/edit-post/header/ellipsis-menu.

[mcsf]

Can't we reuse MenuItemsGroup from @wordpress/components? That would also minimize the stylesheet.

[abotteram]

I tried it, but these menu items require some specific functionality the MenuItemsGroup doesn't have.

[jasonagnew]

Hey @xyfi

I was wondering if you had time if you could provide an example of using the menu. From my understanding, it works like:

wp.editor.registerEllipsisMenuItem( "plugin-namespace/name", {
	'title': "Name",
	'callback': () => {
		// Do Something.
	},
	'icon': () => {
		// Return SVG as react component.
		return (
			<svg>
				<circle cx={50} cy={50} r={10} fill="red" />
			</svg>
		)
	}
} );

Thanks.

[aduth]

Components and elements are not synonymous:

https://reactjs.org/blog/2015/12/18/react-components-elements-and-instances.html

Further, the abstraction of wp.element should be respected here. These isn't strictly a React Component, it's a "WordPress Element Component", or however we'd intend to express this (WPComponent).

[aduth]

Worth pointing out that resetting the Node require cache on every test case is not scalable, and its use should be limited.

[aduth]

Nit: The intent of the validation is not to exclude numbers specifically, it's to exclude non-strings. The test description could be improved to reflect this.

[aduth]

Sorry, I know I recommended this, but import semantics are preferred in place of these globals:

import { sprintf } from '@wordpress/i18n';

Related discussion: https://wordpress.slack.com/archives/C02QB2JS7/p1519839851000186

[aduth]

FWIW, while it probably has little impact here in that this warning is very infrequently logged, but I expect sprintf being not a language-native function is considerably less performant than a simple string concatenation.

[aduth]

1. Misalignment of parameter descriptions
2. Inconsistency of square brackets applied to argument name

[abotteram]

I'm not sure what you are referring to with #2. Isn't that how all optional parameters are documented in Gutenberg?

[aduth]

This exact behavior is optimized in classnames via its accepting of objects as arguments:

const buttonClassName = classnames( 'edit-post-plugins__button', {
	'has-icon': Icon,
	'active': pluginActive,
} );

[aduth]

Modifiers are encouraged to have some form of modifier prefix, typically is- (or above has- is fine, too):

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#naming

[aduth]

This class name does not align with the recommended standards:

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#naming

It should be: edit-post-plugins__button

The name "plugins" alone being perhaps prone to conflict is a good indication that the folder name itself could do for some improvement, as most all folders of component should be clear in its intent standalone. "Plugins" is quite ambiguous.

[aduth]

You're mutating the registered value of menuItem here. Are you intending to? Shouldn't that occur at registration-time, not in the render? Renders shouldn't have side effects.

[aduth]

This should be prefixed editPost., not editor., given the file location.

[aduth]

Is passing menuItemId as an argument necessary if it's available as a property of the settings object?

[aduth]

Is the discrepancy between this registration's returning of null and block's undefined return; intentional? If it's an issue that block's registration should return a non-undefined value, is there a plan to follow-through with a pull request to keep block registration consistent as well?

[aduth]

To be quite frank, I'm fundamentally at odds with the direction of this API. It appears to be modeled after block registration. Unlike a block type which is largely a unit of data, what we have here is chiefly concerned with UI interaction. We already have well-defined patterns for dealing with UI: namely, component composition. The needs of a plugin are not too much unlike the needs for a block's editable interface. Even for things like rendering into disparate parts of the application, we have tools like <SlotFill /> to achieve this behavior via portaling. You can see this in a block's implementation of its inspector controls. These render into a sidebar, but are still defined as descendents of the block's edit implementation. We shouldn't limit ourselves to thinking about componentization as mapping one-to-one with the DOM. As it relates to a plugin there's no reason we couldn't or shouldn't aggregate everything it renders into a single top-level component (think an <App /> component).

Something like what @mtias describes in #4484 (comment) is more what I'd have in mind. It doesn't even need to be this complex, if we want to automate managing of conditions on behalf of the plugin author, similar to what's being explored for blocks in #5029. If there's shared metadata describing a plugin, it seems fine to have a single registration point. But beyond that, I don't see the need for separate registration APIs for each part of the UI, and it conflicts with working patterns that have been established to date.

Registration could be as simple as...

registerPlugin( 'myplugin', {
	// ... whatever data is not UI-specific

	render() {
		// ... everything UI
	},
}

If we're determined to trudge through with flagging it as experimental and dealing with these problems later, that's fine too I guess. But speaking for myself, I don't approve of these changes.

[omarreiss]

@aduth I can empathize with all of the concerns and preferences raised by you, @gziolo and @mtias and I think I understand them. I just don't have the idea my concern is being understood or addressed by any of you, so I will try to explain again and I hope one of you can address it.

When I click a button in the more menu, it becomes active and triggers a sidebar. The "active" state of that more menu button and the presence of the sidebar are directly linked. When I click to deactivate the button, the sidebar should disappear again. When I click the x button on the sidebar, the button should deactivate. When a plugin is also pinned to the toolbar, we have another element that acts in predictable ways. This behavior is always the same and Gutenberg should be responsible for it, because it ties in directly with the UX of how these elements interact with each other. When I read pseudo code like

{ props.isActive &&
  <Sidebar>Some stuff</Sidebar>
}

I see the opposite. This is making plugins worry about isActive as a condition for filling a certain slot. I just don't see the point of that. I see this lead to duplicate boilerplate for every plugin and a UX which is no longer guaranteed by Gutenberg where it could be. How would you solve that?

[aduth]

See #5029 as a demonstration of this exact situation as it has unfolded for block implementer's in their rendering of inspector controls for selected blocks only. The conditions may be useful in their explicitness, but are not strictly necessary.

[omarreiss]

@aduth Ok, I guess I didn't read well enough there. If we can make that work, I agree with your simplest approach. I can just have one render function with a bunch of fills and managing the context is handled by Gutenberg, plus we even get an extra level of flexibility because that context is still also available for plugins. Awesome.

I guess that means that:

• Sidebar API should be changed to simple slot/fill
• More menu filters could also be replaced with simple slot/fill
• This PR could be replaced with one which just implements the simple API you proposed.

It feels like that would be in line with everything that has been previously discussed. Agree?

[jasmussen]

One reason why it's important to have a More Menu item attached to the sidebar at all times, is that on phones we'll hide every pinned plugin icon in the toolbar, and so if the user is to have a chance to see the sidebar on mobile, they'll invoke it as a modal from the menu.

[abotteram]

It's worth mentioning we are currently working on a new version of the sidebar ánd more menu item API, using the proposed Slot/Fill API (#5430).

[aduth]

Closing in favor of the new direction of #5430 and subsequent work.