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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edit Post: Refactor and expose SidebarMoreMenuItem as part of Plugins API #6086

Merged
merged 7 commits into from Apr 18, 2018

Conversation

@gziolo
Member

gziolo commented Apr 9, 2018

Description

It's time to expose Plugin APIs as final. This is part 2 which is similar to work done in #6031. Those 2 PRs together will contain the final API. Which means it will no longer be hidden behind __experimental namespace 馃帀

Proposed AP changes

Before:

<PluginMoreMenuItem
    name="my-menu-item"
    icon={ Icon }
    target="my-sidebar"
    type="sidebar"
>
    { __( 'My sidebar' ) }
</PluginMoreMenuItem>

After (version 2):

<PluginSidebarMoreMenuItem
    target="my-sidebar"
    icon={ Icon }
>
    { __( 'My sidebar' ) }
</PluginSidebarMoreMenuItem>

Changes explained

This PR tries to align with all other usages of Slot & FIll pattern in the codebase. In general we always have a wrapping component which represents Fill and we pass implementation as children prop. See the following examples:

https://github.com/WordPress/gutenberg/blob/master/blocks/library/paragraph/index.js#L150-L157

<BlockControls key="controls">
	<AlignmentToolbar
		value={ align }
		onChange={ ( nextAlign ) => {
				setAttributes( { align: nextAlign } );
		} }
	/>
</BlockControls>

https://github.com/WordPress/gutenberg/blob/master/blocks/library/heading/index.js#L123-L145

<InspectorControls key="inspector">
	<PanelBody title={ __( 'Heading Settings' ) }>
		<p>{ __( 'Level' ) }</p>
		<Toolbar
			controls={
				'123456'.split( '' ).map( ( level ) => ( {
					icon: 'heading',
					title: sprintf( __( 'Heading %s' ), level ),
					isActive: 'H' + level === nodeName,
					onClick: () => setAttributes( { nodeName: 'H' + level } ),
					subscript: level,
				} ) )
			}
		/>
		<p>{ __( 'Text Alignment' ) }</p>
		<AlignmentToolbar
			value={ align }
			onChange={ ( nextAlign ) => {
				setAttributes( { align: nextAlign } );
			} }
		/>
	</PanelBody>
</InspectorControls>

In this case PluginsMoreMenuGroup plays the same role and works very closely to what BlockControls and InspectorControls do in the Blocks API context. PluginSidebar also works in a similar fashion. I belive that we should be consistent in that regard to make this pattern repeatable between every extensibility potin we introduce.
As effect of this change I also renamed PluginMoreMenuItem to SidebarMoreMenuItem to make it possible to use it also outside of the Plugins menu group. When we introduce pinning for the More Menu items, we will be able to reuse SidebarMoreMenuItem component for Document settings.

Finally, I combined the menu group (Fill) with the menu item to keep the API proposed initially. The only changes introduces visible publicly are as follows:

  • new name PluginSidebarMoreMenuItem to avoid passing type explicitly given that every extension type will have a different handling and propos anyway.
  • name prop is never used thus obsolete at this moment. We can bring it back later when we discover it is necessary.

Known issues

The bottom border isn't applied properly to the plugins menu group because of the way additional div is applied. - fixed with 1237abc.

How Has This Been Tested?

Example ES5 code to paste in the JS console in the browser:

var Button = wp.components.Button;
var PanelBody = wp.components.PanelBody;
var dispatch = wp.data.dispatch;
var Fragment = wp.element.Fragment;
var el = wp.element.createElement;
var __ = wp.i18n.__;
var registerPlugin = wp.plugins.registerPlugin;
var PluginSidebar = wp.editPost.PluginSidebar;
var PluginSidebarMoreMenuItem = wp.editPost.PluginSidebarMoreMenuItem;

function SidebarContents() {
	var onClose = dispatch( 'core/edit-post' ).closeGeneralSidebar;
	return (
		el(
			PanelBody, {},
			el( 'p', {}, __( 'Here is the sidebar content!' ) ),
			el( Button, { isPrimary: true, onClick: onClose }, __( 'Close' ) ),
		)
	);
};

function MyPlugin() {
	return (
		el( Fragment, {},
			el(
				PluginSidebar,
				{
					name: 'my-sidebar',
					title: 'My sidebar'
				},
				el( SidebarContents, {} )
			),
			el(
				PluginSidebarMoreMenuItem,
				{
					target: 'my-sidebar',
					icon: 'smiley'
				},
				__( 'My sidebar' )
			)
		)
	);
};

registerPlugin( 'my-plugin', {
	render: MyPlugin,
} );

Screenshots (jpeg or gifs if applicable):

screen shot 2018-04-09 at 16 22 36
screen shot 2018-04-10 at 12 56 06

Checklist:

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

@gziolo gziolo added the Extensibility label Apr 9, 2018

@gziolo gziolo self-assigned this Apr 9, 2018

@youknowriad

Here's some feedback from someone that haven't been involved in the API work. So more like a user feedback.

Why is the Slot API tied to the opening/closing of targets?

Why do we need a "Group" and an "Item" slot for the More Menu.

For me a simpler alternative could be something like this:

<PluginMoreMenu>
      <wp.components.Group>
                <wp.components.Item onClick={ () => dispatch( 'core/edit-post' ).activateTarget( 'sidebar', 'my-sidebar') }>
                                { __( 'My sidebar' ) }
                 </wp.components.Item>
      </wp.components.Group>
</PluginMoreMenu>
  • A unique slot for the more menu PluginMoreMenu
  • Inside it I can compose any UI component (group, item) and onClick, I can trigger any data module API :)
*/
import { createContext, createHigherOrderComponent } from '@wordpress/element';
const MoreMenuContext = createContext( { onMenuClose: noop } );

This comment has been minimized.

@gziolo

gziolo Apr 10, 2018

Member

Needs to be updated with whatever Dropdown exposes in renderContent .

@gziolo gziolo added this to To do in Extensibility via automation Apr 10, 2018

@gziolo gziolo added this to the 2.7 milestone Apr 10, 2018

@gziolo gziolo requested review from WordPress/gutenberg-core, Xyfi, atimmer and IreneStr Apr 10, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 11, 2018

After more thinking, I think the "Group" Slots make sense to be able to extend separate groups:

PluginsMoreMenuGroup to add your plugin to the plugins slot
EditorMoreMenuGroup to add a new editor mode
etc...

But this means the PluginMoreMenuItem is useless. At the moment this component implies we're going to open a Sidebar, a Fullscreen or a modal.. But People will start asking how to add Menu items that don't open anthing but just trigger something (like spellcheck or something else).

I propose we remove it since it's not flexible enough and just rely on the data module and the generic UI components:

<wp.components.MenuItem onClick={ () => dispatch( 'core/edit-post' ).activateTarget( 'sidebar', 'my-sidebar') }>
         { __( 'My sidebar' ) }
</wp.components.MenuItem>

cc @atimmer @Xyfi

@atimmer

This comment has been minimized.

Member

atimmer commented Apr 11, 2018

@youknowriad The reason we want to 'connect' a menu item with a sidebar is to handle all the active state logic in Gutenberg. In your example, the plugin would have to start managing the active state of the menu item. Once the menu items can be pinned this becomes more code. And that code has to be repeated among dozens of plugins.

I think we should also have a generic MenuItem component that can be used to trigger spell-checks and such. But if someone wants to have a menu item that opens a sidebar, SidebarMoreMenuItem works well for this purpose.

@atimmer

I've looked at the code, and from a compositional React standpoint I like the direction of this PR. It makes the components a lot more composable.

The thing I don't like is that is a regression in usability of the API. It is harder to explain and requires to learn more concepts that are not absolutely necessary to learn at that point. It has an easy remedy though: Re-create the PluginMoreMenuItem as a wrapper around PluginsMoreMenuGroup and the MySidebarMoreMenuItem. It should have the same API as the experimental PluginMoreMenuItem.

return Component;
return {

This comment has been minimized.

@atimmer

atimmer Apr 11, 2018

Member

I like this refactor.

const PluginsMoreMenuGroup = ( { children } ) => (
// Plugin's context needs to be forwarded to make it work properly with Dropdown.
<PluginContext.Consumer>

This comment has been minimized.

@atimmer

atimmer Apr 11, 2018

Member

Is this due to the React <16.3 context API, or just Slot/Fill in general?

This comment has been minimized.

@gziolo

gziolo Apr 11, 2018

Member

I think it's because Slot/Fill. Dropdown uses slot, and there is also another slot for PluginsMoreMenuGroup in the menu. At some point, they aren't part of the same tree in the virtual DOM. It works for Sidebar without any issues.

@aduth, any ideas? You know much better Slot & Fill API.

This comment has been minimized.

@aduth

aduth Apr 11, 2018

Member

Yeah, it would be where we're using the "non-virtualized" Slot/Fill, where we're creating a new React tree (via ReactDOM.render) and thus losing the context. I'd love if we could move toward dropping this toward always using the portal implementation, or at least swapping it to serve as the default.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 11, 2018

@atimmer Handling its own state is fine for me :) I can even see plugins opening modals without handling state. If you open a modal for instance, you don't care about the state of the button. It's behind the modal anyway.

It feels like something specific to be handled by the plugin itself. The generic MenuItem already exists I believe.

If we want to keep it, I would change its name though, because It has nothing to do with the MoreMenu. Maybe more something like MenuItemPluginTargetToggle or something :) (I know it's a really bad name but you get the idea)

@atimmer

This comment has been minimized.

Member

atimmer commented Apr 11, 2018

@youknowriad I think the majority of modal cases will be handled by the screen takeover.

I am mostly worried about forcing every plugin developer to write this: https://github.com/WordPress/gutenberg/blob/7ba5324ca279288b5da80a6b9424a325b2ff8def/edit-post/components/header/sidebar-more-menu-item/index.js. And while you and I can write it very efficiently, not everyone can or wants to put the effort in. It is really hard to forget how much you have to understand to come up with that file. While most people that are building plugins only know PHP.

We can discuss naming the thing differently.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 11, 2018

The createHigherOrderComponent is useless, it's a bug and should be removed from this PR. The withSelect and withDispatch are basic APIs I'm expecting any WP plugin developers would understand them and use them, their usage is not limited to the MenuItem.

I expect most menu items would be just something like

const SidebarMoreMenuItem = () => (
	<MenuItem
		icon={ 'my-plugin-icon' }
		onClick={ () => dispatch( 'core/edit-post' ).openTarget( 'modal', 'my-plugin' ) }
	>
		My Plugin
	</MenuItem>
);
@gziolo

This comment has been minimized.

Member

gziolo commented Apr 11, 2018

The createHigherOrderComponent is useless, it's a bug and should be removed from this PR. The withSelect and withDispatch are basic APIs I'm expecting any WP plugin developers would understand them and use them, their usage is not limited to the MenuItem.

It isn't a bug. It's a conscious decision to make it backward compatible with what we had before. In particular target="my-sidebar" param gets plugin's name prepended magically and becomes target="my-plugin/my-sidebar" :) Sure, we can remove it, but then you will have to explicitly provide the full target name.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 11, 2018

@youknowriad, there is toggle implemented in this PR so it would have to become:

const SidebarMoreMenuItem = ( { closeMoreMenu } ) => (
	<MenuItem
		icon={ 'my-plugin-icon' }
		onClick={ () => {
			dispatch( 'core/edit-post' ).toggleTarget( 'modal', 'my-plugin/my-modal' );
			closeMoreMenu(); // needs to be passed exposed somehow
		}
	>
		My Plugin
	</MenuItem>
);

The other thing missing, I added in here, is closing the More Menu dropdown.

@atimmer

This comment has been minimized.

Member

atimmer commented Apr 11, 2018

@youknowriad I think we've had this discussion before: #4484 (comment).

Rehashing some important points from that discussion:

The concept of a plugin is something that Gutenberg owns. When a sidebar is active all the relevant parts of the Gutenberg UI should behave. Gutenberg should be opinionated about which interaction models are the preferred way to do things.

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.

(From #4484 (comment))

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 11, 2018

The thing I don't like is that is a regression in usability of the API. It is harder to explain and requires to learn more concepts that are not absolutely necessary to learn at that point. It has an easy remedy though: Re-create the PluginMoreMenuItem as a wrapper around PluginsMoreMenuGroup and the MySidebarMoreMenuItem. It should have the same API as the experimental PluginMoreMenuItem.

The only difference between the old and new approach is that you need to explicitly wrap your component with PluginsMoreMenuGroup component or any other group that is going to be exposed in a follow-up PRs like Settings or Tools. Those other groups can be extended using hooks at the moment, but this should be changed and align with Plugins group. The only thing you need to learn here is that you need to pick a component where your menu item has to be placed - PluginsMoreMenuGroup - and what type of component(s) should be put inside - SidebarMoreMenuItem. This is much more flexible as you can combine a few different items together:

<PluginsMoreMenuGroup>
	<SidebarMoreMenuItem
		name="my-menu-item"
		icon="simley"
		target="my-sidebar"
	>
		{ __( 'My sidebar' ) }
	</SidebarMoreMenuItem>
	<MenuItem
		icon="my-plugin-icon"
		onClick={ () => dispatch( 'core/edit-post' ).openTarget( 'modal', 'my-plugin' ) }
	>
		My Modal
	</MenuItem>
</PluginsMoreMenuGroup>

@gziolo gziolo moved this from To do to In progress in Extensibility Apr 11, 2018

@atimmer

This comment has been minimized.

Member

atimmer commented Apr 11, 2018

Do people really have to understand Slot/Fill when they use these APIs? If we make it simple enough they don鈥檛 have to. They can, and understanding it will make them better developers. But the highest level APIs don鈥檛 require it:

  • I want settings for my block -> <InspectorControls> (馃 maybe this should be called <BlockSidebar> / <BlockSettings>
  • I want a sidebar -> <PluginSidebar>
  • I want a way to open my sidebar -> <PluginMoreMenuItem> (馃 maybe this should be called <MoreMenuOpenSidebar> / <MoreMenuShowPlugin(UI) )

Underlying these we can have beautifully composed components. I think that is the best of both worlds: Easy to get into and teaching best practices when people dive in.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 11, 2018

@atimmer I think we all agree about this.

The PluginMoreMenuItem is still confusing though. It has two roles a UI role (rendering the menu item) and a behavior role (toggling a target) which means it will be hard to find a good name for it.

I don't think we should keep MoreMenu in its name because this component renders a menu item that can be shown in any menu, it has no relation with the MoreMenu since it doesn't contain any fill.

I still think this component is useless, basically just a combination of the raw wp.components.MenuItem and a higher order component providing two props toggleTarget( type, name ) and and isOpened prop. I'm fine keeping it, just questioning its usefulness because it feels too opinionated for me.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 18, 2018

@gziolo Let's rename SidebarMoreMenuItem to ToggleTargetMenuItem and merge this to included in 2.7. or maybe just PluginMenuItem. I think we should remove the More` from the name

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 18, 2018

My current proposal looks as follows:

<PluginSidebarMoreMenuItem
    target="my-sidebar"
    icon={ Icon }
>
    { __( 'My sidebar' ) }
</PluginSidebarMoreMenuItem>

Finally, I combined the menu group (Fill) with the menu item to keep the API proposed initially. The only changes introduce visible publicly are as follows:

  • new name PluginSidebarMoreMenuItem to avoid passing type explicitly given that every extension type will have a different handling and props anyway.
  • name prop is never used thus obsolete at this moment. We can bring it back later when we discover it is necessary.
);
```
#### Props
##### name
A string identifying the menu item. Must be unique for every menu item registered within the scope of your plugin.

This comment has been minimized.

@gziolo

gziolo Apr 18, 2018

Member

I didn't find any reason to keep it for the time being. We can bring it back as soon there is use case for it.

</PluginContext.Consumer>
),
'withPluginContext'
),

This comment has been minimized.

@mcsf

mcsf Apr 18, 2018

Contributor

Is there a plan to reuse this? (In theory, withPluginContext does sound like something we'd expect @wordpress/plugins to offer.) Otherwise, I worry it may hurt readability of the module when compared with

const PluginsSidebarMoreMenuItem = ( { children, isSelected, icon, onClick } ) => (
  <PluginContext.Consumer>
    { ( { pluginName } ) => (
      <PluginsMoreMenuGroup>
);
export default compose(
createHigherOrderComponent(

This comment has been minimized.

@aduth

aduth Apr 18, 2018

Member

Should we offer this as a proper export of @wordpress/plugins : withPluginContext ?

This comment has been minimized.

@gziolo

gziolo Apr 18, 2018

Member

It might make sense to do it, because we use a very similar logic in PluginSidebar.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 18, 2018

Resharing a full example with JSX to give a full picture:

const { Fragment } = wp.element;
const { PluginSidebar, PluginSidebarMoreMenuItem } = wp.editPost;
const { registerPlugin } = wp.plugins;

const Component = () => (
	<Fragment>
		<PluginSidebarMoreMenuItem
			target="sidebar-name"
			icon="smiley"
		>
			My Sidebar
		</PluginSidebarMoreMenuItem>
		<PluginSidebar
			name="sidebar-name"
			title="My Sidebar"
		>
			Content of the sidebar
		</PluginSidebar>
	</Fragment>
);

registerPlugin( 'plugin-name', {
	render: Component,
} );
@gziolo

This comment has been minimized.

Member

gziolo commented Apr 18, 2018

@aduth @mcsf - I refactored to use withPluginContext with 4916982. It shouldn't have any impact on other things.

@mcsf

mcsf approved these changes Apr 18, 2018

Nice work 馃憤

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 18, 2018

Thanks @mcsf for improving docs 馃憤

@gziolo gziolo merged commit f78b390 into master Apr 18, 2018

2 checks passed

codecov/project 43.9% (+0.06%) compared to 48ce55a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Extensibility automation moved this from In progress to Done Apr 18, 2018

@gziolo gziolo deleted the update/plugins-more-menu-group branch Apr 18, 2018

Xyfi added a commit that referenced this pull request Apr 24, 2018

nuzzio added a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018

Edit Post: Refactor and expose SidebarMoreMenuItem as part of Plugins鈥
鈥 API (WordPress#6086)

* Edit Post: Refactor and expose SidebarMoreMenuItem as part of Plugins API

* Edit Post: Update SidebarMoreMenuItem to work closer to the previous version

* Edit Post: Fix border issues in MoreMenu introduced with Slot & Fill

* Docs: Update Plugins API documentation

* Edit Post: Include the menu group inside plugins menu item

* Plugins: Introduce withPluginContext HOC

* withPluginContext: minor copy updates to docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment