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

IreneStr · 2018-01-31T14:09:54Z

Description

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

The API looks like this:

wp.editPost.registerSidebar( 
	"my-plugin/my-sidebar", 
	{
		title: "Name",
		render: () => { return <div>My sidebar</div>;}
	} 
);

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

How Has This Been Tested?

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

Remarks

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]. When refreshing the page with the sidebar opened, the same error will appear.
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). I've removed the mobile middleware in the edit-post folder. It would be great if someone can double check whether I didn't break anything related to that.
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.

For more comments, see #4109

jasmussen · 2018-02-08T09:09:09Z

I think I said this on the last PR, but I'll say it again: nice work — I think developers will love this API.

Keep in mind I can only speak for the UI and user experienceon this, so take any thoughts from me in that vein.

The principles are these:

A plugin can register a sidebar
This sidebar can be opened from the More menu, if the user clicks the item there, or a toolbar icon button, if the sidebar is pinned
A toolbar icon is pinned, but can be unpinned. I previously suggested that a plugin can't itself pin the icon, but the user has to do this. However per the consensus in our recent slack chats, let's double back on that, and let a plugin add a toolbar icon if it wants to. But the user should still be able to unpin that icon (see the star bookmark button in older mockups).
A sidebar with an unpinned icon can be opened from the More menu item.
A pinned icon is a single-color inline SVG icon

As also mentioned in a previous PR, not all of these principles have to be part of THIS PR. It's totally fine that we address these later on. But I'm explicitly stating these so we can have some agreement as to where this should end up.

If we agree this is the approach to pursue and this will be possible and desirable, then I think UI wise this PR is good to go after two quick visual fixes. The plugin sidebar should look a bit more like the standard sidebar:

Right now the plugin sidebar has a bigger font size in the title (should be font-size: 100%;), the titlebar is 56px, should be 50px, and the close icon is right-aligned, should have a 10px right margin. In other words this is how it should look:

With those tiny visual tweaks, and an agreement as to where to go next, 👍 👍 from me on the visuals and interaction!

jasmussen · 2018-02-08T11:27:25Z

Another quick note I forgot to mention — the behavior for the sidebar hiding and not restoring when you resize the screen has regressed from master. We should ideally restore it like we do in master.

We also need to be sure that the mobile experience is good, the modal sidebar system has regressed also:

But modals on mobile need imminent polish regardless, as part of #4082, so perhaps that doesn't need to be addressed in this PR. The thing is — due to how WordPress handles fixed and sticky toolbars at various breakpoints, and due to the need to handle mobile scroll bleed (don't scroll content underneath the modal when you're swiping, and also don't lose your place when you open a modal), it's difficult to get the modals to behave well here. I know @youknowriad has a component that takes some of the vinegar out.

jorgefilipecosta · 2018-02-08T11:40:35Z

edit-post/store/selectors.js

+ * @return {string}        Active plugin sidebar plugin
+ */
+export function getActivePlugin( state ) {
+	return getPreference( state, 'activeSidebarPanel', {} ).plugins;


Minor: in the state tree we save as "plugins", but the selector uses "Plugin". Should we allow more than one plugin to be active? If yes the selector should be renamed to getActivePlugins if not the property should be renamed to plugin.

@jorgefilipecosta That's a good question. I think because this function is sidebar specific (using 'activeSidebarPanel' in the getPreference function), there could only be one plugin active at any given time. I'll rename the property accordingly.

jorgefilipecosta · 2018-02-08T11:59:25Z

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

+export default connect(
+	( state ) => {
+		return {
+			plugin: getActivePlugin( state ),


It looks like this will always return null (and we will see the 'No matching plugin sidebar found for plugin ""'), because the store being queried here is not the store that contains the active ActiveEditorPanel preference.
We can confirm that by adding console.log( 'PluginsPanel' , state ); before the return.
We can correct that by specifying the store key:

export default connect( ( state ) => { return { plugin: getActivePlugin( state ), }; }, { onClose: closeGeneralSidebar, }, undefined, { storeKey: 'edit-post' } )( withFocusReturn( PluginsPanel ) );

jorgefilipecosta

It looks like the changes, to the mobile sidebar, hiding/persisting behavior are good, nice job in that refactoring 👍

jorgefilipecosta · 2018-02-08T12:13:02Z

edit-post/store/defaults.js

-		publish: false,
+	editorMode: 'visual',
+	viewportType: 'desktop', // 'desktop' | 'mobile'
+	activeGeneralSidebar: null, // null | 'editor' | 'plugins'


I think here we should set activeGeneralSidebar to 'editor', otherwise, we are introducing a behavior change because when reloading the page without preferences stored in localStorage no sidebar will be active.

gziolo

Thank you for your work on this PR. It looks like we are pretty close to have it merged.

I think we still need to clarify how the public API should look like. I left some question regarding renderSidebar and getSidebarSettings methods exposed. It seems like registerSidebar would be a better fit here. I'm wondering what's your take on this and why it has changed in this new PR.

I played with the code locally and I saw some issues:

I think @jorgefilipecosta found the reason of this issue. We need to fix it before merging this PR. The screenshot I shared reveals also the issue with the renderSidebar method - which registers the sidebar every time it's going to be executed. This needs to be optimized.

I have a feeling that it might be possible to reuse or extract more functionality out of the existing sidebar components. This might be done in the follow-up PRs, we just should identify what would be the best way to move forward.

Another thing to think about in the future (not for this PR) is if we could handle the global settings and block settings as sidebar plugins, too. This would allow us further simplify the way we handle state because we wouldn't have to worry about two competing sidebars which want to be displayed in the same area.

Let's make sure there are no unexpected behaviors and come up with the plan how to continue work on this feature.

gziolo · 2018-02-08T10:11:38Z

edit-post/api/sidebar.js

+ */
+function activateSidebar( name ) {
+	store.dispatch( openGeneralSidebar( 'plugins' ) );
+	store.dispatch( setGeneralSidebarActivePanel( 'plugins', name ) );


Would be it possible to dispatch one action named after the function (activateSidebar) to update all reducers that are involved in this state update?
Isn't meant to be dispatched directly inside the menu component in the future? What is the reason for having it outside of the component's lifecycle?

This will be addressed in a next iteration

gziolo · 2018-02-08T10:12:44Z

edit-post/api/index.js

@@ -0,0 +1,4 @@
+export {
+	renderSidebar,
+	getSidebarSettings,


Can you give an example of the use case where getSidebarSettings would be useful for plugin developers? It not used in the code as part of this API. I don't see also any comments in the PRs description. It would be nice to have a better overview on how this API might work.

The exposure of getSidebarSettings was a remnant of some debugging. I'll remove it from the API. I cannot think of a reason to leave it exposed. When someone thinks of a use case it can be exposed again.

gziolo · 2018-02-08T10:18:20Z

edit-post/api/sidebar.js

+		return null;
+	}
+
+	settings = applyFilters( 'editor.registerSidebar', settings, name );


I think we have the same logic in blocks.registerBlockType. I'm afraid this needs to be further discussed. The thing is that it happens after validation process is done, so you can update all settings however you want and it won't be validated again. It seems like this should happen before the validation begins.

This will be addressed in a next iteration

gziolo · 2018-02-08T10:54:28Z

edit-post/api/index.js

@@ -0,0 +1,4 @@
+export {
+	renderSidebar,


In the previous version, we had registerSidebar exposed in the public API. What was the reason to use renderSidebar instead?

gziolo · 2018-02-08T11:05:53Z

edit-post/api/sidebar.js

+ *
+ * @return {void}
+ */
+export function renderSidebar( name, settings ) {


When I call this method twice for the same name, I see the following error on the console:

Sidebar my-plugin/my-sidebar is already registered.

This is exposed as public API. I assume this is what would be executed from the menu, so it shouldn't register the same sidebar every time a user clicks on the menu item.

gziolo · 2018-02-08T11:26:59Z

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

+				/>
+			</div>
+			<div className="editor-plugins-panel__content">
+				{ render() }


This won't work properly in the case where render property will contain Component class. It should follow React guidelines here: Composing Components.

<div> <RenderComponent /> </div>

On second thought I don't think we need to change that. It is perfectly fine as long as we make it clear that you need to return WPElement from the function, similar to what createElement produces:

class MyComponent extends Component { render() { return <div>My Component</div> } } const render = () => <MyComponent />;

gziolo · 2018-02-08T11:27:47Z

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

+		render,
+	} = getPluginSidebar( plugin );
+	return (
+		<div


This component is almost identical to <Sidebar /. It would be nice to reuse it here. See:
https://github.com/WordPress/gutenberg/blob/master/edit-post/components/sidebar/index.js#L24

Since <Header/> is nested inside <Sidebar/>, this would be something for a next iteration as well (see comment below https://github.com/WordPress/gutenberg/pull/4777/files/3cdf86f520f8f379b50eeed62872e24fb7516b76#r167880008)

gziolo · 2018-02-08T11:30:36Z

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

+			tabIndex="-1">
+			<div className="editor-plugins-panel__header">
+				<h3>{ title }</h3>
+				<IconButton


There is also <Header /> component which looks similar to what we have here: https://github.com/WordPress/gutenberg/blob/master/edit-post/components/sidebar/header.js#L41. For instance it contains an identical close button. Is it possible to make those components more flexible so we could use them in here, too?

The close button is the only similarity. The header in sidebar/header.js solely consists of three buttons. The header component may be generalized to fit both headers, but that's something for a next iteration.

gziolo · 2018-02-08T11:49:15Z

edit-post/components/layout/index.js

+import { closeGeneralSidebar, closePublishSidebar } from '../../store/actions';
+import PluginsPanel from '../../components/plugins-panel/index.js';
+
+function GeneralSidebar( { openedGeneralSidebar } ) {


This might be inlined inside the component that is consuming it. If it is going to be reused in other places it should be moved to its own file.

gziolo · 2018-02-08T11:50:01Z

edit-post/components/layout/index.js

+			{ publishSidebarOpen && <PostPublishPanel onClose={ onClosePublishSidebar } /> }
+			{
+				openedGeneralSidebar !== null && <GeneralSidebar
+					onCloseGeneralSidebar={ onCloseGeneralSidebar }


Both props passed down aren't propagated to the child components.

jorgefilipecosta · 2018-02-08T12:33:14Z

We are persisting the active plugin panel, so if when we reload because of some reason the sidebar is not available (e.g: plugin was uninstalled) we immediately enter into an error state. As an improvement when loading the preferences from localStorage we should check if the sidebar is available before showing the error.

jorgefilipecosta · 2018-02-08T12:41:57Z

Thank you a lot for the work in this PR, I left some suggestions about possible improvements/bugfixes but generally, it looks like we are getting close to a merge :)

gziolo · 2018-02-14T14:37:04Z

edit-post/store/index.js

@@ -8,7 +8,6 @@ import { registerReducer, withRehydratation, loadAndPersist } from '@wordpress/d
 */
 import reducer from './reducer';
 import enhanceWithBrowserSize from './mobile';
-import applyMiddlewares from './middlewares';
 import { BREAK_MEDIUM } from './constants';


We don't need ./middlewares file anymore if we stop using it here.

@youknowriad can you confirm we can remove it completely?

yes, it's a local dependency if the mobile middleware is gone we can safely remove it. but how is the mobile Middleware replaced right now? Sorry didn't review in depth?

The need for mobile middleware was removed because of a logic was changed. As a side effect:

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.

I think it is ok to remove that behavior as the reopen from mobile to desktop was not an initial requirement. And we can try to propose a solution for that if required after this PR is merged.

gziolo · 2018-02-19T15:48:46Z

When I register my sidebar and activate it, then refresh the page I still see this error about missing plugin. I think it needs to be fixed, because it is expected that this sidebar shouldn't be there, so this error is only confusing for the user:

jasmussen · 2018-02-19T15:49:57Z

While I really want this to get merged in, I would object to the merger until these UI issues are fixed. Specifically:

The plugin sidebar header is too tall compared to the post settings sidebar header
The font is larger compared to the post settings sidebar header
The X close button is offset compared to the post settings sidebar header

My primary concern is that we merge this in and work on addressing the UI glitches falls through the cracks, forgotten mostly because you'd only ever see it if you also installed a plugin that took advantage of this new API, which very few would do.

jorgefilipecosta · 2018-02-19T16:13:45Z

The improvements I referred in the last review are addressed, thank you for applying the fixes.
The error @gziolo referred is caused by the plugin sidebar not being available when we reload. I referred this problem and said it may be future work because it probably happens when plugins are removed, but I was not aware of all the impact it may have in other cases e.g: plugin renaming sidebars.
So if we can add a verification to check if the sidebar is available when loading the preferences from local storage instead of showing the error, solving the reload issues @gziolo referred it would be nice.
Thank you for your contribution 👍

IreneStr · 2018-02-20T15:22:29Z

@jasmussen My bad. I was under the impression that I had fixed those UI things when I did a CSS review and removed duplicate CSS, which wasn't the case. I've just pushed more CSS changes to address the issues you raised.

@jorgefilipecosta @gziolo I'll address your feedback tomorrow.

jasmussen · 2018-02-21T07:14:38Z

edit-post/components/plugins-panel/style.scss

@@ -12,6 +12,10 @@

    h3 {
      margin: 0;
+      font-weight: 400;
+      color: inherit;


Haven't had time to check, but should the font size also be addressed? The font size should be 13px, and you get that from the $default-font-size variable here https://github.com/WordPress/gutenberg/blob/master/edit-post/assets/stylesheets/_variables.scss#L7

@jasmussen The current font size is 13px. Or do I need to set it explicitly for some reason?

Ah right, sorry, wasn't able to test. No that looks good. Though feel free to set the font weight to 600 so it's bold, like an active tab. Otherwise 👍 👍

gziolo · 2018-02-21T13:10:49Z

I added a few commits to remove the sidebar state with error messages. Let's tackle it separately in a way that is less confusing for the users, but at the same time helpful for site owners so they could get proper feedback.

At this moment when there is no matching sidebar registered by Redux has active plugin stored, we stop rendering sidebar. Let's iterate on this in the follow-up PR.

gziolo · 2018-02-21T13:45:03Z

There is one known issue. Steps to reproduce:

register the sidebar and activate it
reload the page
it gets closed as expected
if then we register the sidebar again and activate it, it does not show up as expected

This is not likely that user will get into such flow, so I think we can fix it in a follow-up PR.

@IreneStr thanks for working on this one. It looks solid and we can proceed. We agreed that it's better to merge it in the current shape and address all the comments in the smaller targeted PRs. ✨

IreneStr added 3 commits January 31, 2018 11:48

Make it possible for plugins to render a sidebar

a69476e

Fix sidebar styling

f5586c9

Remove unused mobile middleware

26c088e

IreneStr mentioned this pull request Jan 31, 2018

Add an API to add a plugin sidebar #4109

Closed

3 tasks

aduth added this to the Feature Complete milestone Jan 31, 2018

aduth 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 31, 2018

IreneStr added 4 commits February 1, 2018 10:07

Fix editor mode switching

d221bd9

Fix editorMode switch test

d31b21c

Fix documentation

b69794b

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

1db6a05

gziolo requested review from jorgefilipecosta and jasmussen February 7, 2018 15:07

Change @returns into @return

3cdf86f

jorgefilipecosta reviewed Feb 8, 2018

View reviewed changes

gziolo reviewed Feb 8, 2018

View reviewed changes

IreneStr added 5 commits February 12, 2018 15:42

Add storeKey to make the plugins panel connect to the correct store

fbf1225

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

f30e950

Rename 'plugins' to 'plugin' in selector

380a125

Set editor as default activeGeneralSidebar

cdfc5a6

Unexpose getSidebarSettings

872ddcc

IreneStr added 3 commits February 13, 2018 14:42

Remove duplicate scss and remove renderSidebar function

271db7c

Fix unittests

e116f96

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

6bed5f5

gziolo reviewed Feb 14, 2018

View reviewed changes

youknowriad mentioned this pull request Feb 14, 2018

Question: Integrate 3rd party editor (page builder) #4960

Closed

gziolo mentioned this pull request Feb 15, 2018

Blocks: Move filter for registering block types before validation happens #5076

Merged

3 tasks

gziolo mentioned this pull request Feb 19, 2018

Add More Menu Item Api #4484

Closed

3 tasks

jasmussen mentioned this pull request Feb 20, 2018

Design User Experience for Switching Between Page Builders/Alternate Editing Scenarios #1375

Closed

Fix header height, font and close button

2388b60

jasmussen reviewed Feb 21, 2018

View reviewed changes

gziolo added 4 commits February 21, 2018 12:28

Update index.js

c417b7e

Edit Post: Remove no longer used getPluginSidebar

0570733

Edit Post: Remove no longer used imports from PluginsPanel

29beb82

Edit Post: Update Layout to handle missing plugin properly

ab02dff

Edit Post: Remove obsolete blank line

85d6ff1

gziolo merged commit 25de703 into WordPress:master Feb 21, 2018

atimmer deleted the add/api-add-plugin-sidebar-2 branch February 22, 2018 09:21

gziolo mentioned this pull request Feb 23, 2018

Framework: Add viewport module (data module experiments, proof-of-concept) #5206

Merged

2 tasks

davidulus mentioned this pull request Feb 25, 2018

Kompatibilita s Gutemberkem skaut/skautis-integration#60

Open

gziolo mentioned this pull request Mar 7, 2018

Edit Post: Refactor state handling for the sidebar #5435

Merged

3 tasks

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

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

Conversation

IreneStr commented Jan 31, 2018 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Remarks

jasmussen commented Feb 8, 2018 • edited Loading

jasmussen commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta left a comment • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo 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

Choose a reason for hiding this comment

gziolo Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IreneStr Feb 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

jorgefilipecosta commented Feb 8, 2018 • edited Loading

jorgefilipecosta commented Feb 8, 2018

gziolo Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Feb 19, 2018

jasmussen commented Feb 19, 2018

jorgefilipecosta commented Feb 19, 2018

IreneStr commented Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Feb 21, 2018

gziolo commented Feb 21, 2018

IreneStr commented Jan 31, 2018 •

edited

Loading

jasmussen commented Feb 8, 2018 •

edited

Loading

jorgefilipecosta Feb 8, 2018 •

edited

Loading

jorgefilipecosta left a comment •

edited

Loading

jorgefilipecosta Feb 8, 2018 •

edited

Loading

gziolo Feb 8, 2018 •

edited

Loading

IreneStr Feb 13, 2018 •

edited

Loading

jorgefilipecosta commented Feb 8, 2018 •

edited

Loading

gziolo Feb 14, 2018 •

edited

Loading

IreneStr commented Feb 20, 2018 •

edited

Loading