Add an API to add a plugin sidebar #4109

atimmer · 2017-12-20T12:27:27Z

Description

This adds an API that plugins can use to add sidebars.

The API looks like this:

// Registering a sidebar:
wp.editor.registerSidebar( "name", { title: "Name", render: () => { return <div>My sidebar</div>; } } );

// Activating a sidebar:
wp.editor.activateSidebar( "name" );

The screen takeover that is mentioned in #3330 can be implemented in a very similar way. The plugins menu should then probably be sorted alphabetically. And it should have both registered sidebars as registered screens.

How Has This Been Tested?

I've created a PR on Yoast SEO to test this with. Yoast/wordpress-seo#8531 shows how the API can be used.

Screenshots (jpeg or gifs if applicable):

Types of changes

New API.

Checklist:

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

youknowriad · 2017-12-20T14:09:05Z

Thanks for working on this @atimmer this is something we know we want as an extension point in Gutenberg :)

Here are some quick remarks without going too deep into the code and testing yet.

From the mockups in Native Gutenberg Extensibility Overview #3330 I think we want this to be a separate sidebar and not reuse the sidebar used for the block/document settings
We need a way to "pin" this plugin to the header, which means we need to specify an icon for the sidebar and we need to add the UI bits for this behavior (fine to do later if want to iterate on this). Best shown here Native Gutenberg Extensibility Overview #3330 (comment)
For now this API lives in wp.editor.registerSidebar. Is sidebar the correct term here? Also, I think once we move the edit-post to its own module and keep the editor as generic module to build "wp block editors" see Framework: Extract "edit-post" to its own module #3649 This API may be moved to this newly created module since its something specific to this screen.

Great to see a PR for this

jasmussen · 2017-12-20T14:42:56Z

Niiice work!

I was able to hackily test by checking out the Yoast origin and branch, then pasting this to my console:

wp.editor.registerSidebar( "my-plugin/my-custom-sidebar", { title: "Name", render: () => { return 'My sidebar'; } } );

wp.editor.activateSidebar( "my-plugin/my-custom-sidebar" );

It works as advertised! Very very very cool, thank you for working on this. A few thoughts:

As this API exists now, it's kind of between two chairs. It's not quite the sidebar system envisioned in #3330 as it replaces the contents of the Settings sidebar, as opposed to create a new sidebar that can co-exist alongside it. But it's also not quite able to inject a custom metabox in the post settings sidebar, as it just replaces all of the existing ones.

However, it does feel like this lays down the skeleton to potentially accomplish both. Is that a correct interpretation?

Aside from that, I'd echo Riad's thoughts — it'd be really nice to see the API mature into being able to register an entirely new sidebar. But slick work so far!

Lazy sidenote: I always fumble when I check out a branch on a fork, my Git-Fu is just weak — is it possible to branch off of master? 😇 — I know, I'm lazy, and it's okay if you prefer to work in the fork, we're still grateful for the contributions.

atimmer · 2017-12-20T15:02:13Z

@jasmussen I don't have access to push to the main Gutenberg repo 😉

atimmer · 2017-12-20T15:04:24Z

@jasmussen @youknowriad I didn't notice that it should be a separate sidebar. I will change the PR to make it a separate sidebar. This definitely lays down a skeleton to start allowing all sorts of registrations.

jasmussen · 2017-12-20T15:11:53Z

I don't have access to push to the main Gutenberg repo 😉

Man, we gotta address that.

jasmussen · 2017-12-20T15:18:58Z

I didn't notice that it should be a separate sidebar. I will change the PR to make it a separate sidebar.

🌟 Definitely let me know if any of the mockups are unclear!

This definitely lays down a skeleton to start allowing all sorts of registrations.

🎉

afercia · 2017-12-21T12:57:53Z

Happy to see this 🙂 From an a11y perspective I don't see issues in this specific implementation. More general issues related to the interaction with the sidebar(s) were mentioned in other conversations and also in #3330. See also #469

gziolo · 2017-12-22T12:21:29Z

@atimmer great work exploring how we can add a new item to the ellipsis menu and replace Advanced settings in the sidebar. I second what @youknowriad said, this is what we want from the UX perspective, but it needs some further thinking in terms of underlying technical implementation. We need to make sure we expose API that is going to work even when we decide to move the sidebar from the editor module or when we decide to convert sidebar to a dialog or something else.

For now this API lives in wp.editor.registerSidebar. Is sidebar the correct term here? Also, I think once we move the edit-post to its own module and keep the editor as generic module to build "wp block editors" see #3649 This API may be moved to this newly created module since its something specific to this screen.

I would be in favor of using wp.hooks.addFilter to both inject the item in the Ellipsis menu and inject new panel in the "sidebar" (name to be defined) :)

atimmer · 2017-12-22T14:47:18Z

@youknowriad

Why wouldn't sidebar be the correct term? This is the same term that Gutenberg uses internally. Another term we could use is aside, like the HTML element [1]. I do think that sidebar is better because it is a term that people know. It is instantly obvious what it is supposed to do.

[1] aside:

(...) represents a portion of a document whose content is only indirectly related to the document's main content. (...)

I have no objections to the move towards edit-post. This API is highly specific to editing a post.

Another topic that makes me think of is exploring programmable API discoverability. In a way doing: if ( wp && wp.editPost && wp.editPost.registerSidebar ) { is feature detection in and of itself. So that could be enough already.

I will address the other feedback using code changes once #4119 is merged.

@gziolo

If we ever (re)move the sidebar, we deprecate the API. Having more semantic APIs makes it much more clear to people when something is not used/removed. Having this API just not do anything in the customizer shouldn't be a problem. Or it could even be rendered in the left sidebar, but I digress.

About the filter part. In this current iteration, a call to registerSidebar does multiple things (with reasons why the API does it):

Sets everything up so a plugin sidebar can be activated. (The main purpose of this API)
Adds a menu item to the ellipsis menu. (An opinionated default to prevent plugin authors from having to invent an interaction model for their plugin.)
[Not yet implemented in PR] Adds a way to pin sidebars to the toolbar. (An opinionated way for users to customize their UI, they way they want.)

These could also be 3 filters. But then the whole "opinionated default to prevent plugin authors from having to invent an interaction model for their plugin" is not there anymore.

I think we should also have 3 filters in all of these 3 locations. But that is a low-level API we shouldn't require most people to use. wp.editor.registerSidebar is a high-level API. So it can be opinionated. And it can help the person using the API a lot with 'free' UI. The higher-level API has a lot more guardrails that help people using it with how to use it.

Error messages like The "render" property must be specified and must be a valid function. are a lot harder to implement using only filters. We'd have to check the output of the filter and the stack trace of the error doesn't contain the plugin that registered the faulty sidebar. Failing silently is also not very helpful.

aduth · 2018-01-02T20:40:16Z

editor/edit-post/header/ellipsis-menu/index.js

@@ -28,6 +29,8 @@ const element = (
 <ModeSwitcher onSelect={ onClose } />
 <div className="editor-ellipsis-menu__separator" />
 <FixedToolbarToggle onToggle={ onClose } />
+ <div className="editor-ellipsis-menu__separator" />
+ <Plugins onToggle={ onClose } />


This looks odd when no plugins exist:

In these cases, it probably should not be shown (or shown with messaging that no plugins exist).

aduth · 2018-01-02T20:42:31Z

editor/edit-post/header/plugins/index.js

+ * @param {Object} props Props.
+ * @param {Function} props.onSwitch Function to call when a plugin is
+ * switched to.
+ * @param {string} props.activePanel The currently active panel.


Even though the JSDoc documentation differentiates with lower and uppercase differences, and I'd previously made an attempt to respect them, I think we ought to just standardize on always uppercasing, to avoid needing to consistently consult with the docs (or more likely, do it wrong).

This could be a good discussion to have in #core-js. Tentatively I would say that keeping primitives lower case has value for a parser. This is consistent with PHP, where primitives are always lowercase and objects are always spelled the way they are defined in the class statement.

Doesn't block this PR, I think.

aduth · 2018-01-02T20:43:12Z

editor/edit-post/header/plugins/index.js

+ * Internal dependencies
+ */
+import { getSidebars, activateSidebar } from '../../../api/sidebar';
+import { MenuItemsGroup } from '../../../../components';


We should not be reaching into other top-level directories. This should be @wordpress/components (and moved under "WordPress dependencies" comment)

aduth · 2018-01-02T20:43:22Z

editor/edit-post/header/plugins/index.js

+import { getSidebars, activateSidebar } from '../../../api/sidebar';
+import { MenuItemsGroup } from '../../../../components';
+import { getActivePanel, isEditorSidebarOpened } from '../../../store/selectors';
+import { connect } from 'react-redux';


This should be moved under "External dependencies"

aduth · 2018-01-02T20:44:27Z

editor/api/sidebar.js

+ );
+ }
+
+ if ( ! ( 'title' in settings ) || settings.title === '' ) {


Could this be simplified to if ( ! settings.title ) { ?

aduth · 2018-01-02T20:46:04Z

editor/api/sidebar.js

+ *
+ * @param {string} name The name of the sidebar to retrieve.
+ *
+ * @returns {false|Object} The settings object of the sidebar. Or false if the


The "return false if not exists" pattern never quite made sense to me in the context of JavaScript, since we already have two concepts to represent an empty/unset value: null and undefined. I'd rather we return one of these instead, preferably null since we already do this in a number of selectors and it semantically better represents the "explicitly empty" use-case (vs. unset).

Aside: If we did return false, the return value type would be Boolean.

aduth · 2018-01-02T20:47:37Z

editor/api/sidebar.js

+ * @param {string} settings.title The name to show in the settings menu.
+ * @param {Function} settings.render The function that renders the sidebar.
+ *
+ * @returns {Object} The final sidebar settings object.


TIL @returns and @return are equally valid and equivalent JSDoc tags. Noting that we've conventionally used @return elsewhere.

In the WordPress JavaScript documentation standards we go with @returns.

…add/api-add-plugin-sidebar

IreneStr · 2018-01-08T11:23:13Z

editor/api/sidebar.js

+ }
+ if ( ! /^[a-z][a-z0-9-]*\/[a-z][a-z0-9-]*$/.test( name ) ) {
+ console.error(
+ 'Sidebar names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-sidebar'


Missing period

IreneStr · 2018-01-08T11:29:43Z

editor/api/sidebar.js

+ *
+ * @param {string} name The name of the sidebar to retrieve.
+ *
+ * @returns {Object} The settings object of the sidebar. Or false if the


Or null (not false)

IreneStr · 2018-01-08T11:30:17Z

editor/api/sidebar.js

+}
+
+/**
+ * Activates the gives sidebar.


the given sidebar

IreneStr · 2018-01-08T12:14:17Z

editor/api/sidebar.js

+ if ( sidebars[ name ] ) {
+ console.error(
+ 'Sidebar "' + name + '" is already registered.'
+ );


Missing return

This might be intentional to overwrite a plugin?

Minor: Normally we avoid + operator to concatenate strings Maybe we can use Sidebar "${ name }" is already registered..

IreneStr · 2018-01-08T12:17:53Z

editor/store/actions.js

+ * @param {String} plugin The plugin name
+ * @return {Object} Action object
+ */
+export function setActivePlugin( plugin ) {


setActive(Plugin)Sidebar might be a better name, because a plugin can have multiple sidebars.

IreneStr · 2018-01-08T12:20:17Z

editor/store/selectors.js

@@ -97,6 +97,16 @@ export function getActivePanel( state ) {
 return state.panel;
 }

+/**
+ * Returns the current active plugin for the plugin sidebar.


See above: a plugin can have multiple sidebars

IreneStr · 2018-01-08T12:56:25Z

editor/components/plugins-panel/index.js

+ return () => {
+ return <Panel>
+ <PanelBody>
+ { sprintf( __( 'No matching plugin sidebar found for plugin "%s"' ), plugin ) }


Please add translator comment

gziolo · 2018-01-09T13:18:10Z

Why wouldn't sidebar be the correct term? This is the same term that Gutenberg uses internally. Another term we could use is aside, like the HTML element [1]. I do think that sidebar is better because it is a term that people know. It is instantly obvious what it is supposed to do.

The issue is that this API is to use case specific. This PR addresses only the sidebar
but we want to allow 4 different ways for plugins to integrate with Gutenberg, where clicking on the menu item in the ellipsis menu can do any of these:

open a popover menu
do an action (example: copy all post)
open a sidebar
open a new screen

It's explained in depth in #3330. Screens might help here, too:

That's why we would rather see the public API closer to:

wp.editor.registerPlugin( {
    title: "My Plugin",
    icon: svg | img,
    render () {
         return (
            <SidebarPanel>
                <div>My Stuff...</div>
            </SidebarPanel>
        );
    }
});

Please also note that in this example we use <SidebarPanel /> component to decide where the plugin will render. This would leave the decision what to do when plugin gets activated to the implementor.

abotteram · 2018-01-09T13:57:18Z

@gziolo I'm not fond of the idea to determine where the component is rendered based on what component you choose to use. The reason for this is that the plugin has to be rendered in the same place, regardless of the type of plugin.

If we were to go with a single registerPlugin function, I propose we pass an extra parameter defining the type of plugin (ie. "sidebar", "takeover", "modal"). This way we have more control over where to render the actual component.

gziolo · 2018-01-09T20:31:18Z

@gziolo I'm not fond of the idea to determine where the component is rendered based on what component you choose to use. The reason for this is that the plugin has to be rendered in the same place, regardless of the type of plugin.

If we were to go with a single registerPlugin function, I propose we pass an extra parameter defining the type of plugin (ie. "sidebar", "takeover", "modal"). This way we have more control over where to render the actual component.

This is not exactly what I proposed. <SidebarPanel /> or <Takover /> can be implemented using Slot Fil:

Slot and Fill are a pair of components which enable developers to render elsewhere in a React element tree, a pattern often referred to as "portal" rendering. It is a pattern for component extensibility, where a single Slot may be occupied by an indeterminate number of Fills elsewhere in the application.

This approach is used in a few places in Gutenberg. One of the examples is block's inspector controls. They are included in the edit function definition of the block but rendered in practice inside the sidebar. See the button example: https://github.com/WordPress/gutenberg/blob/master/blocks/library/button/index.js#L102.

In my opinion, this is more flexible approach and allows us to skip the definition of the plugin type. In practice, you could even dynamically change plugin type based on the data that might be fetched from elsewhere.

abotteram · 2018-01-10T08:11:50Z

@gziolo Ah, I wasn't aware of the design pattern, thank you for elaborating. For now, I'll keep working on the PR as is, and the plugin API can be revised at a later date. This is something that should be discussed in the Gutenberg chat.

IreneStr · 2018-01-25T10:14:00Z

This PR is ready for review @youknowriad @gziolo @aduth @jasmussen @jorgefilipecosta

Some remarks:

I've added a renderSidebar function that calls a registerSidebar and a activateSidebar function.
When triggering a plugin sidebar from outside Gutenberg (see the Yoast branch mentioned by @atimmer above), there is a timing issue. Sometimes the activation happens before the registration, which results in an error No matching plugin sidebar found for plugin [plugin name].
To test the registration and activation separately, expose activateSidebar and registerSidebar in editor/api/index.js.
Because of we're handling sidebars differently now, mobileMiddleware is no longer needed. Whether the mobile sidebar should be opened is now based on a viewport property in the state (it used to be based on sidebar: 'mobile' in the state).
The sidebar is hidden when you change from desktop to mobile. However, it's not saved that it was opened before, which means it will not be reopened when switching back to desktop.

jasmussen · 2018-01-26T08:22:36Z

Took this for a quick spin, and it looks like it's going to work really well, nice work!

There are a couple of ToDo's for this feature, but probably none that have to happen in this PR — it seems like it would good to wrap this up, merge it in, and then ticket these improvements separately. Including:

The heading bar of the newly registered sidebar is a little taller than the Settings sidebar, and the X to close is offset in comparison
Perhaps it's my lack of skill in this area but I couldn't get an icon to show up, and there's no pinning function. I'm assuming this is something we just add later on, which is fine.
You mention this yourself, but compared to master, the sidebar state management has regressed a little bit. If you have the sidebar open in master, size down the viewport to collapse it and then size it up again, the sidebar is gone.

Nice work!

afercia · 2018-01-26T08:34:11Z

@jasmussen Quick consideration about pinning (I'd agree it should go in a separate issue). Have you considered what users will typically do?:

(I've used random icons, just to get an idea)

Lots of plugins: lots of pinned icons. This should be carefully considered, as it poses usability and accessibility issues.I'd tend to think the pinned icons should have their own separate area somewhere.

jasmussen · 2018-01-26T08:48:37Z

Have you considered what users will typically do?:

Yes.

And you're exactly right — your concept is not unlikely to happen in some situations, and the designer in me screams inside at the thought. But at the same time, I would place this in the same category as top level plugin menus — we want to allow them because they can bring huge value to plugin authors.

But by not pinning these things by default, requiring the user to actively open the plugin from the ellipsis menu and then opting in by explicitly pressing the pin button, hopefully we are not only teaching the user how they can remove those icons again (untoggle the pin), but we may also be limiting the amount of icons there.

At this particular stage in the process, this flow is inspired by how Chrome and Firefox do with their extensions. We believe it will work for us, but we also want to be open to adjusting our assumptions as we try this in practice.

IreneStr · 2018-01-26T13:38:52Z

@jasmussen Thanks for your feedback. I agree merging this PR and iterate over it is the way to go.

Btw, the icons and pinning are part of the ellipsis menu PR that @xyfi has been working on. I'm not sure what's the status of that PR.

Edit: ah, here it is: #4484

youknowriad · 2018-01-26T18:54:02Z

Sorry for the conflicts, If you have trouble rebasing this, feel free to ask me. (We moved edit-post to a separate module).

jasonagnew · 2018-01-29T12:33:11Z

Hey @IreneStr

If you've got time I was wondering when using the renderSidebar it activates the sidebar.

wp.editor.renderSidebar( "plugin-namespace/name", {
	'title': "Name",
	render: () => {
		return (
			<div>
				My Sidebar
			</div>
		)
	}
} );

Does this mean you're meant to call renderSidebar only when you want to show the sidebar? If that's the case if you reload the page when your sidebar is open it would trigger an error about sidebar not being available as it's not yet registered. I know originally it used to have registerSidebar and activateSidebar so that case it would be registered on load, then call activate any time you want to show the sidebar. Maybe I'm missing something here.

Thanks for your time.

IreneStr · 2018-01-31T13:56:29Z

@jasonagnew My apologies for my late reply. Using the current structure, we're trying to work towards uniform APIs for opening a sidebar, doing a screen takeover, etc. However, you're correct that reloading with an open plugin sidebar will cause an error at this moment. That's one of the issues that still needs to be addressed.

IreneStr · 2018-01-31T14:13:38Z

This PR has been replaced by #4777
Because of the massive amount of merge conflicts due to moved and split files, I created a new branch on which I recommited my changes in the right places.

aduth · 2018-01-31T14:15:05Z

Closed in favor of #4777

Add an API to add a plugin sidebar

f3d3daf

youknowriad requested review from aduth, mtias, gziolo and jasmussen December 20, 2017 14:09

aduth reviewed Jan 2, 2018

View reviewed changes

Alexander Botteram added 6 commits January 3, 2018 10:33

Merge branch 'master' into add/api-add-plugin-sidebar

2864974

Applied minor codestyle changes to sidebar.js

b4fa154

Applied minor codestyle changes to plugins header

ba05cd8

Merge branch 'master' of https://github.com/WordPress/gutenberg into …

506fd15

…add/api-add-plugin-sidebar

Fixed broken dependency after merging wordpress/gutenberg

9caaf8e

Initial attempt at rendering plugins in a separate sidebar

53579b3

IreneStr reviewed Jan 8, 2018

View reviewed changes

Removed accidentally added IDE file

9428aa9

IreneStr reviewed Jan 8, 2018

View reviewed changes

Alexander Botteram added 2 commits January 9, 2018 16:33

Initial changes

7241609

removed .idea

8870121

IreneStr added 9 commits January 22, 2018 14:51

Rename viewMode to viewportType

fd057e8

Merge branch 'master' into add/api-add-plugin-sidebar

af1cc84

Add SET_VIEWPORT_TYPE to reducers

c7daa63

Fix unittests part 1

a5ffe6c

Fix unittests part 2

ec870c3

Remove unneeded padding

b34991d

Remove mobile middleware

06950bd

Close sidebar when switching to mobile view

fce3ddd

Unexpose activateSidebar

02cc074

Fix codestyle

0072e66

mtias added this to the Feature Complete milestone Jan 26, 2018

mtias added Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Extensibility The ability to extend blocks or the editing experience labels Jan 26, 2018

IreneStr mentioned this pull request Jan 31, 2018

Add an API to add a plugin sidebar (new) #4777

Merged

3 tasks

aduth closed this Jan 31, 2018

aduth removed this from the Feature Complete milestone Jan 31, 2018

abotteram deleted the add/api-add-plugin-sidebar branch February 1, 2018 07:42

Add an API to add a plugin sidebar #4109

Add an API to add a plugin sidebar #4109

Conversation

atimmer commented Dec 20, 2017

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

youknowriad commented Dec 20, 2017

jasmussen commented Dec 20, 2017

atimmer commented Dec 20, 2017

atimmer commented Dec 20, 2017

jasmussen commented Dec 20, 2017

jasmussen commented Dec 20, 2017

afercia commented Dec 21, 2017

gziolo commented Dec 22, 2017 • edited Loading

atimmer commented Dec 22, 2017

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

atimmer Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IreneStr Jan 8, 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

gziolo commented Jan 9, 2018 • edited Loading

abotteram commented Jan 9, 2018

gziolo commented Jan 9, 2018

abotteram commented Jan 10, 2018

IreneStr commented Jan 25, 2018 • edited Loading

jasmussen commented Jan 26, 2018

afercia commented Jan 26, 2018

jasmussen commented Jan 26, 2018

IreneStr commented Jan 26, 2018 • edited Loading

youknowriad commented Jan 26, 2018

jasonagnew commented Jan 29, 2018

IreneStr commented Jan 31, 2018

IreneStr commented Jan 31, 2018

aduth commented Jan 31, 2018

gziolo commented Dec 22, 2017 •

edited

Loading

atimmer Jan 3, 2018 •

edited

Loading

IreneStr Jan 8, 2018 •

edited

Loading

gziolo commented Jan 9, 2018 •

edited

Loading

IreneStr commented Jan 25, 2018 •

edited

Loading

IreneStr commented Jan 26, 2018 •

edited

Loading