Extensibility: Plugin sidebar api #5430

abotteram · 2018-03-06T09:18:13Z

Description

Also props to @IreneStr.

Usage:

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

// Separate component to avoid "unnamed component" warning.
const Component = () => {
	return (
		<Fragment>
			<PluginSidebar name="sidebar" title="Sidebar title">
				<h2>Some content!</h2>
			</PluginSidebar>
			<PluginSidebar name="second-sidebar" title="Sidebar 2 title">
				<h2>Some more content!</h2>
			</PluginSidebar>
		</Fragment>
	);
};

// Register the plugin using the new API
registerPlugin( {
	name: "plugin-namespace", // Should start with a lowercase letter and can only contain lowercase letters, numbers and dashes.
	render: Component,
} );

Important: Due to a bug in the current API you should open and close the editor sidebar before executing the following line.

Then to activate the sidebar execute the following line in the console:
wp.data.dispatch( 'core/edit-post' ).openGeneralSidebar( 'plugin', 'plugin-namespace/sidebar' );
or
wp.data.dispatch( 'core/edit-post' ).openGeneralSidebar( 'plugin', 'plugin-namespace/second-sidebar' );

Known issues:

Providing an unknown namespace/sidebarName id opens a white sidebar.
A way to activate the sidebar without needing the console.

How Has This Been Tested?

Tested in editor, using console.
Added unit tests.

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 has proper inline documentation.

gziolo · 2018-03-06T10:03:15Z

edit-post/components/plugins-panel/index.js

@@ -44,7 +45,7 @@ function PluginsPanel( { onClose, plugin } ) {
 				/>
 			</div>
 			<div className="edit-post-plugins-panel__content">


It seems like PluginsPanel should be converted into the slot.

Yes, that's the plan.

gziolo · 2018-03-06T10:06:25Z

edit-post/api/plugin.js

+					const Context = plugin.context;
+					return (
+						<Context.Provider key={ plugin.name } value={ plugin.name }>
+							{ plugin.render() }


Shouldn't you wrap this with <PluginSidebar /> to activate the fill?

PluginSidebar is the component the plugin developer uses in the registerPlugin API call. When rendering the plugins you don't know what kind of ui-element it is (Sidebar, screen takeover, etc).

gziolo · 2018-03-06T10:08:39Z

edit-post/api/plugin.js

+				{ map( plugins, plugin => {
+					const Context = plugin.context;
+					return (
+						<Context.Provider key={ plugin.name } value={ plugin.name }>


This is also a good place to put logic to make sure that sidebar is only rendered when a given plugin is active.

…debar component

…ontext

gziolo · 2018-03-07T13:14:43Z

edit-post/api/components/context.js

+/**
+ * External dependencies
+ */
+import PropTypes from 'prop-types';


We are not using PropTypes at all in Gutenberg. It probably wasn't removed from package.json.

gziolo · 2018-03-07T13:16:18Z

edit-post/api/index.js

-	registerSidebar as __experimentalRegisterSidebar,
-	activateSidebar as __experimentalActivateSidebar,
-} from './sidebar';
+	registerPlugin,


To repeat what I said above. Let's mark it as experimental for one release.

gziolo · 2018-03-07T13:39:19Z

edit-post/api/components/test/context.js

+
+			const tree = renderer.toJSON();
+
+			expect( tree.children[ 0 ] ).toEqual( 'plugin-namespace' );


Gutenberg has jest-enzyme bundled, it is possible to use snapshots here and mount helper. See: https://github.com/WordPress/gutenberg/blob/master/docs/testing-overview.md#snapshot-testing.

…separate sidebar layout component

gziolo · 2018-03-07T13:44:20Z

edit-post/components/layout/index.js


 function GeneralSidebar( { openedGeneralSidebar } ) {
 	switch ( openedGeneralSidebar ) {
 		case 'editor':
 			return <Sidebar />;
 		case 'plugin':
-			return <PluginsPanel />;
+			return <PluginSidebarSlot />;


It would be nice to avoid using the slot part in the name of the component. It should be known only internally.

gziolo · 2018-03-07T13:45:48Z

edit-post/components/layout/index.js

@@ -111,6 +107,7 @@ function Layout( {
 					openedGeneralSidebar={ openedGeneralSidebar } />
 			}
 			<Popover.Slot />
+			<PluginFills />


This component might be renamed to PluginArea or something like this to better reflect its role.

gziolo · 2018-03-07T13:47:48Z

edit-post/components/plugin-sidebar/index.js

+ */
+const SLOT_NAME = 'PluginSidebar';
+
+class SidebarErrorBoundary extends Component {


This component should be put in its own file. In the long run, it should be generalized to support or types of plugins.

gziolo · 2018-03-07T13:50:25Z

edit-post/components/plugin-sidebar/index.js

+	constructor( props ) {
+		super( props );
+
+		if ( typeof props.name !== 'string' ) {


Do we need to include this validation in here? I don't feel like anything bad would happen if the non-string value would be provided. It would be converted to a string and it wouldn't be possible to activate such sidebar.

It's a matter of providing accurate feedback to the developer I think?

It feels like this kind of behavior should be enabled only in development env. I don't know, @mtias and @aduth - do we have some kind of strategy in place?

We shouldn't throw.

#5430 (comment)

…/'-prefix in sidebar identifier

aduth · 2018-03-15T18:47:11Z

Take a look at 6540b05 as what I had in mind. Let me know what you think.

Specifically, it's leveraging Slot/Fills, generating the name of the intended visible plugin's sidebar as the name of the slot, so only it would be shown.

Use createElement

Apparently the convention we established (see: Popover)

abotteram

A few points I would like to address in regards to @aduth's changes:

It's allowed to have 2 sidebars with the same name to be rendered, without any feedback why only one is showing.
It's not enforcing the plugin-sidebar naming conventions.

I still think it's wise to wrap the plugin sidebars in a separate error boundary, because we should protect the editor from crashing because of plugin code as much as possible.

abotteram · 2018-03-15T18:56:02Z

edit-post/store/selectors.js

-
-	return activeGeneralSidebar.startsWith( 'plugin-sidebar/' );
+	const activeGeneralSidebar = getActiveGeneralSidebarName( state );
+	return !! activeGeneralSidebar && ! isEditorSidebarOpened( state );


This was actually what I was trying to prevent because it doesn't allow any more sidebar types. But I guess that's a concern for later.

It's the nice thing about a selector is that it abstracts away these underlying implementation-detail concerns.

abotteram · 2018-03-15T20:44:40Z

plugins/api/index.js


-const registry = PluginRegistry.getInstance();
+	settings.name = name;
+	settings.sidebar = {};


This doesn't do anything

aduth · 2018-03-16T12:06:09Z

I don't disagree, but those things are not strictly necessary for an MVP, and can be introduced and discussed in a separate follow-up pull request.

I do have thoughts on them though, if you'd like a few more rounds of back-and-forth 😉

aduth

One more thing: The sidebar doesn't look great on mobile. It appears the default sidebars apply a 100% width in a separate selector that this is not benefitting from.

Aside: We need to consolidate our concept of what a sidebar is, so we're not addressing each individually like this.

The className for the plugin sidebar was changed in 644b7e8. But this was missed in one location. This commit changes the class there too.

aduth

👍

mcsf · 2018-03-16T15:49:01Z

edit-post/components/layout/index.js

@@ -19,6 +19,7 @@ import {
 } from '@wordpress/editor';
 import { withDispatch, withSelect } from '@wordpress/data';
 import { compose } from '@wordpress/element';
+import { PluginArea } from '@wordpress/plugins';


Should we add an entry to .eslintrc.js?

gutenberg/.eslintrc.js

Lines 77 to 80 in 10df23c

{

selector: 'ImportDeclaration[source.value=/^viewport$/]',

message: 'Use @wordpress/viewport as import path instead.',

},

Should we add an entry to .eslintrc.js?

Added in a1de97b.

mcsf · 2018-03-16T16:10:57Z

plugins/README.md

+    </Fragment>
+);
+
+wp.plugins.__experimental.registerPlugin( 'plugin-names', {


This is outdated, wp.plugins.registerPlugin is available. (PR description is also outdated.)

This is outdated, wp.plugins.registerPlugin is available. (PR description is also outdated.)

I missed this. I assumed we were still exporting as __experimental. I'm fine with "unexperimental"-izing it at this point.

This is outdated, wp.plugins.registerPlugin is available. (PR description is also outdated.)

Updated in 853655c.

mcsf · 2018-03-16T16:14:11Z

bin/build-plugin-zip.sh

@@ -99,7 +99,7 @@ zip -r gutenberg.zip \
 	blocks/library/*/*.php \
 	post-content.js \
 	$vendor_scripts \
-	{blocks,components,date,editor,element,hooks,i18n,data,utils,edit-post,viewport}/build/*.{js,map} \
+	{blocks,components,date,editor,element,hooks,i18n,data,utils,edit-post,viewport,plugins}/build/*.{js,map} \


Tangent: Perhaps by reading webpack.config.js, it'd be nice if we didn't have to manually keep the build script, the ESLint config, etc. in sync.

Tangent: Perhaps by reading webpack.config.js, it'd be nice if we didn't have to manually keep the build script, the ESLint config, etc. in sync.

Yes, this has become an increasingly frequent source of friction.

--> #5664, for starters

abotteram · 2018-03-17T15:07:30Z

@mcsf Completely agree, but these are tooling issues. Have you created issues for the points you are bringing up?

atimmer

Looks like the tooling issues are resolved (or worked on). Giving my own 👍 here and then merging.

Alexander Botteram added 2 commits March 2, 2018 15:56

Initial implementation of slot-fill sidebar API

142f7b1

Attempt to implement createContext api for plugin slot fill API

65af7e6

abotteram mentioned this pull request Mar 6, 2018

Add More Menu Item Api #4484

Closed

3 tasks

gziolo self-requested a review March 6, 2018 09:58

Fixed error on start up

0e921fe

gziolo reviewed Mar 6, 2018

View reviewed changes

Soean added the [Status] In Progress Tracking issues with work in progress label Mar 6, 2018

Alexander Botteram added 4 commits March 6, 2018 14:13

Implemented way to pass props from plugin API via context to PluginSi…

2b022f7

…debar component

Finalize first pass of registerPlugin API

78623ad

Remove create-react-context

0beaa4b

Merge branch 'master' into add/slot-fill-sidebar-api

cbfe6dc

gziolo mentioned this pull request Mar 7, 2018

Edit Post: Refactor state handling for the sidebar #5435

Merged

3 tasks

Alexander Botteram added 5 commits March 7, 2018 11:21

Removed old sidebar and refactored the way plugin context is handled

75e35ba

Improved withPluginContext HOC to prevent usage of cloneElement

1ae8632

Implemented tests for registerPlugin

0d3d03d

Added test for plugin/context components, revamped naming of passed c…

789e675

…ontext

Fixed eslint issues

08f6e7b

gziolo reviewed Mar 7, 2018

View reviewed changes

Alexander Botteram added 2 commits March 7, 2018 14:39

Refactored plugin-sidebar to use withSelect/withDispatch and created …

ffe0870

…separate sidebar layout component

Removed PropTypes

c55eaaf

gziolo reviewed Mar 7, 2018

View reviewed changes

aduth added the [Feature] Extensibility The ability to extend blocks or the editing experience label Mar 7, 2018

Alexander Botteram and others added 6 commits March 14, 2018 17:51

Added slot to pluginsidebar fill for consistency

02a5924

Moved styles.scss under Internal dependencies

687e25c

removed unnecessary id from hidden div for plugin fills

5f8beaa

Removed getPluginUIComponent API function in favor of 'plugin-sidebar…

b435d4b

…/'-prefix in sidebar identifier

Don't throw error on invalid PluginSidebar props

90d0af8

Plugins: Refactor plugin UI registration as Slot/Fill

6540b05

aduth added 3 commits March 15, 2018 14:51

Plugins: Remove unused code

f127727

Plugins: Represent plugin render as the component it is

cdc51c5

Use createElement

Plugins: Revert plugin sidebar slot name to PluginSidebar

5488d06

Apparently the convention we established (see: Popover)

abotteram commented Mar 15, 2018

View reviewed changes

aduth added 2 commits March 16, 2018 08:15

Plugins: Remove unused error boundary styles

9ee20f1

Plugin: Remove unused sidebar settings

ce9b374

aduth requested changes Mar 16, 2018

View reviewed changes

atimmer added 2 commits March 16, 2018 14:14

Fix plugin sidebar with on mobile

a322b70

The className for the plugin sidebar was changed in 644b7e8. But this was missed in one location. This commit changes the class there too.

Merge branch 'master' into add/slot-fill-sidebar-api

d751d50

atimmer added this to the 2.5 milestone Mar 16, 2018

Merge branch 'master' into add/slot-fill-sidebar-api

10df23c

aduth approved these changes Mar 16, 2018

View reviewed changes

mcsf reviewed Mar 16, 2018

View reviewed changes

mcsf mentioned this pull request Mar 16, 2018

Build: Dynamically pick JS/CSS build files for ZIP gen #5664

Merged

3 tasks

aduth added 2 commits March 16, 2018 13:50

Documentation: Remove experimental flag from plugins

853655c

Testing: Include plugins folder in ESLint import rule

a1de97b

atimmer approved these changes Mar 19, 2018

View reviewed changes

atimmer merged commit 3ced7fb into WordPress:master Mar 19, 2018

atimmer deleted the add/slot-fill-sidebar-api branch March 19, 2018 08:25

atimmer mentioned this pull request Mar 26, 2018

Publicize: Adding to Gutenberg Extensibility for Publicize Support #5795

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensibility: Plugin sidebar api #5430

Extensibility: Plugin sidebar api #5430

abotteram commented Mar 6, 2018 •

edited

Loading

gziolo Mar 6, 2018 •

edited

Loading

abotteram Mar 6, 2018

gziolo Mar 6, 2018

abotteram Mar 6, 2018

gziolo Mar 6, 2018

gziolo Mar 7, 2018

gziolo Mar 7, 2018

gziolo Mar 8, 2018

gziolo Mar 7, 2018

gziolo Mar 7, 2018

gziolo Mar 7, 2018

gziolo Mar 7, 2018

gziolo Mar 7, 2018

abotteram Mar 7, 2018

gziolo Mar 8, 2018 •

edited

Loading

aduth Mar 14, 2018

aduth commented Mar 15, 2018

abotteram left a comment

abotteram Mar 15, 2018

aduth Mar 16, 2018

abotteram Mar 15, 2018

aduth commented Mar 16, 2018 •

edited

Loading

aduth left a comment

aduth left a comment

mcsf Mar 16, 2018

aduth Mar 16, 2018

mcsf Mar 16, 2018

aduth Mar 16, 2018

aduth Mar 16, 2018

mcsf Mar 16, 2018

aduth Mar 16, 2018

mcsf Mar 16, 2018

abotteram commented Mar 17, 2018 •

edited

Loading

atimmer left a comment


		const tree = renderer.toJSON();

		expect( tree.children[ 0 ] ).toEqual( 'plugin-namespace' );

	{
	selector: 'ImportDeclaration[source.value=/^viewport$/]',
	message: 'Use @wordpress/viewport as import path instead.',
	},

Extensibility: Plugin sidebar api #5430

Extensibility: Plugin sidebar api #5430

Conversation

abotteram commented Mar 6, 2018 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

gziolo Mar 6, 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

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

gziolo Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Mar 15, 2018

abotteram left a 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

aduth commented Mar 16, 2018 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

aduth left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abotteram commented Mar 17, 2018 • edited Loading

atimmer left a comment

Choose a reason for hiding this comment

abotteram commented Mar 6, 2018 •

edited

Loading

gziolo Mar 6, 2018 •

edited

Loading

gziolo Mar 8, 2018 •

edited

Loading

aduth commented Mar 16, 2018 •

edited

Loading

abotteram commented Mar 17, 2018 •

edited

Loading