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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edit Post: Refactor and expose PluginSidebar as final API #6031

Merged
merged 6 commits into from Apr 10, 2018

Conversation

5 participants
@gziolo
Member

gziolo commented Apr 6, 2018

Description

It's time to expose Plugin APIs as final. This PR refactors all the existing Sidebar components to share the same set components and removes duplicated custom CSS styles applied to <PluginSidebar />. There are no changes to the public API of <PublicSidebar /> component, other than it is going to be available under wp.editPost.PluginSidebar. I think this one works great and perfectly aligns with how <BlockControls /> or <InspectorControls /> work with blocks edit method.

There were a few changes introduced internally to increase code reuse and better split responsibilities between components:

  • <Sidebar /> becomes the component which handles Slot & Fill logic. Each component is always rendered as Fill but is guarded with a conditional check which ensures that it is rendered only when it is active.
  • Editor sidebars where refactored to use the same Slot & Fill behavior to align with plugins.
  • <SettingsHeader /> component was extracted with the logic that handles tabs for Block and Document settings.
  • <BlockInspectorPanel /> component was converted to <BlockSidebar />.
  • <PostSettings /> component was converted to < DocumentSidebar />.
  • <PluginSidebar /> was refactored to use already existing components rather than its own implementation. <SidebarLayout /> was removed together with its styles.

How Has This Been Tested?

Example plugin to test:

const { Button, PanelBody } = wp.components;
const { dispatch } = wp.data;
const { Fragment } = wp.element;
const { __ } = wp.i18n;
const { registerPlugin } = wp.plugins;
const { PluginSidebar } = wp.editPost;
const { PluginMoreMenuItem } = wp.editPost.__experimental;

const Icon = (
	<svg width="100%" height="100%" viewBox="0 0 100 100">
		<circle cx="50" cy="50" r="40" stroke="black" strokeWidth="3" fill="red" />
	</svg>
);
const SidebarContents = () => {
	const onClose = dispatch( 'core/edit-post' ).closeGeneralSidebar;
	return (
		<PanelBody>
			<p>{ __( 'Here is the sidebar content!' ) }</p>
			<Button isPrimary={ true } onClick={ onClose }>{ __( 'Close' ) }</Button>
		</PanelBody>
	);
};
const MyPlugin = () => {
	return (
		<Fragment>
			<PluginSidebar name="my-sidebar" title="My sidebar">
				<SidebarContents />
			</PluginSidebar>
			<PluginMoreMenuItem
				name="my-menu-item"
				icon={ Icon }
				target="my-sidebar"
				type="sidebar"
			>
				{ __( 'My sidebar' ) }
			</PluginMoreMenuItem>
		</Fragment>
	);
};

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

ES5 code to paste into JS console:

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 PluginMoreMenuItem = wp.editPost.__experimental.PluginMoreMenuItem;

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(
				PluginMoreMenuItem,
				{
					name: 'my-menu-item', 
					icon: 'smiley',
					target: 'my-sidebar',
					type: 'sidebar'
				},
				__( 'My sidebar' )
			)
		)
	);
};

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

Screenshots

Document

screen shot 2018-04-06 at 11 35 59

Block

screen shot 2018-04-06 at 11 36 09

Plugin

screen shot 2018-04-06 at 11 46 04

Types of changes

Refactoring applied. There shouldn't be any noticeable changes in the UI.

Checklist:

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

@gziolo gziolo self-assigned this Apr 6, 2018

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

@gziolo gziolo modified the milestones: 2.7, 2.6 Apr 6, 2018

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

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

@gziolo gziolo changed the title from Edit Post: Expose PluginSidebar as final API to Edit Post: Refactor and expose PluginSidebar as final API Apr 6, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 6, 2018

Judging purely from those screenshots, everything looks superb and solid. Very nice.

Can you elaborate on how to test? I tried pasting the example plugin into the console, but got a "Uncaught SyntaxError: Unexpected token <" error.

withSelect( ( select ) => ( {
activeSidebarName: select( 'core/edit-post' ).getActiveGeneralSidebarName(),
} ) ),
ifCondition( ( { activeSidebarName, name } ) => activeSidebarName === name ),

This comment has been minimized.

@gziolo

gziolo Apr 6, 2018

Member

Actually, it'd trigger less re-renders when isActive is returned from withSelect.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 6, 2018

Can you elaborate on how to test? I tried pasting the example plugin into the console, but got a "Uncaught SyntaxError: Unexpected token <" error.

I provided also ES5 version of the plugin which you can paste in the JS console.
Next you go to the More Menu and My Sidebar item should be there :)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 6, 2018

Here's a GIF:

sidebars

Everything about this is solid. Love it. Looks and works and feels right. Ship it!

Can't wait to see the icon pinning feature 🌟

@Xyfi

This comment has been minimized.

Contributor

Xyfi commented Apr 9, 2018

There is something wrong with the mobile view:
another_brave_new_world____ _local_wordpress_dev_ _wordpress
Both the 'x' buttons close the sidebar.

Also @aduth had the idea to reuse the sidebar API for the document and block sidebars as well, instead this PR introduces two new sidebars (BlockSidebar and DocumentSidebar). Is this something we still want?

All in all a good PR, but the mobile view needs to be addressed.

@Xyfi

Xyfi requested changes Apr 9, 2018 edited

The mobile view needs to be fixed. See comment above.

@atimmer

atimmer approved these changes Apr 9, 2018 edited

Having given this a quick look, that general ideas look great. I'll leave it to @Xyfi to review and test the code in more detail.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 9, 2018

Also @aduth had the idea to reuse the sidebar API for the document and block sidebars as well, instead this PR introduces two new sidebars (BlockSidebar and DocumentSidebar). Is this something we still want?

We could do it, although it works exactly the same at the moment, but without explicitly using registerPlugin method. BlockSidebar and DocumentSidebar work almost exactly the same as PluginSidebar, but provide their own customized implementation.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 9, 2018

@Xyfi this issue with movers exists also in the master branch. I think it is a regression introduced in one of the recent PRs.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 9, 2018

Opened #6081 to address issues with the overlapping block controls.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 9, 2018

@Xyfi, I think I fixed all visual issues on mobile:

screen shot 2018-04-09 at 22 05 41

it aligns with Document settings:

screen shot 2018-04-09 at 22 06 53

@Xyfi

This comment has been minimized.

Contributor

Xyfi commented Apr 10, 2018

We could do it, although it works exactly the same at the moment, but without explicitly using registerPlugin method. BlockSidebar and DocumentSidebar work almost exactly the same as PluginSidebar, but provide their own customized implementation.

If it works exactly the same, doesn't that mean there is quite some duplicate code there? Also I think that these lines clutter the layout file a bit.

@Xyfi

Xyfi approved these changes Apr 10, 2018

@Xyfi Xyfi merged commit bd2556c into master Apr 10, 2018

2 checks passed

codecov/project 44.59% (-0.03%) compared to 410c387
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@gziolo gziolo deleted the update/sidebar-refactor branch Apr 10, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 10, 2018

If it works exactly the same, doesn't that mean there is quite some duplicate code there?

I guess there is some duplication of code, but we also save some lines because we don't need to use registerPlugin inside editPost. It still uses the same Slot & Fill as plugins do for sidebars which is the most important part here. To use registerPlugin we would have to make PluginSidebar much more flexible to support tabs instead of title and to provide a different label for accessibility. We can revisit it later if you think it is worth it.

Also I think that these lines clutter the layout file a bit.

Agreed. I had to put those fills somewhere. I didn't find a better way to do it. :)

@mtias

This comment has been minimized.

Contributor

mtias commented Apr 18, 2018

Great to see all the work and refinement behind these APIs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment