Skip to content
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

Add an API to add a plugin sidebar #4109

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f3d3daf
Add an API to add a plugin sidebar
atimmer Dec 20, 2017
2864974
Merge branch 'master' into add/api-add-plugin-sidebar
Jan 3, 2018
b4fa154
Applied minor codestyle changes to sidebar.js
Jan 3, 2018
ba05cd8
Applied minor codestyle changes to plugins header
Jan 3, 2018
506fd15
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
Jan 3, 2018
9caaf8e
Fixed broken dependency after merging wordpress/gutenberg
Jan 3, 2018
53579b3
Initial attempt at rendering plugins in a separate sidebar
Jan 5, 2018
9428aa9
Removed accidentally added IDE file
Jan 8, 2018
7241609
Initial changes
Jan 9, 2018
8870121
removed .idea
Jan 9, 2018
cc0b124
Updated actions, reducers and selectors
Jan 10, 2018
b3aa610
finished implementing new actions, reducers and selectors
Jan 10, 2018
c7b36b3
Fixed last build errors
Jan 10, 2018
e4b9df3
Merge branch 'master' into add/api-add-plugin-sidebar
xyfi Jan 11, 2018
693e5a8
Add renderSidebar function
IreneStr Jan 12, 2018
04d6e2b
Add renderSidebar function
IreneStr Jan 17, 2018
aa3c84b
Remove console.logs and unneeded argument
IreneStr Jan 18, 2018
ed6ea39
Alter class name to enable plugin sidebar-specific styling
IreneStr Jan 19, 2018
3af2240
Unexpose activateSidebar and registerSidebar
IreneStr Jan 22, 2018
10529c8
Fix console error syntax and remove unnecessary error
IreneStr Jan 22, 2018
fd057e8
Rename viewMode to viewportType
IreneStr Jan 22, 2018
af1cc84
Merge branch 'master' into add/api-add-plugin-sidebar
IreneStr Jan 22, 2018
c7daa63
Add SET_VIEWPORT_TYPE to reducers
IreneStr Jan 23, 2018
a5ffe6c
Fix unittests part 1
IreneStr Jan 23, 2018
ec870c3
Fix unittests part 2
IreneStr Jan 24, 2018
b34991d
Remove unneeded padding
IreneStr Jan 25, 2018
06950bd
Remove mobile middleware
IreneStr Jan 25, 2018
fce3ddd
Close sidebar when switching to mobile view
IreneStr Jan 25, 2018
02cc074
Unexpose activateSidebar
IreneStr Jan 25, 2018
0072e66
Fix codestyle
IreneStr Jan 25, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions editor/api/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export {
registerSidebar,
activateSidebar,
} from './sidebar';
107 changes: 107 additions & 0 deletions editor/api/sidebar.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/* eslint no-console: [ 'error', { allow: [ 'error' ] } ] */

/* External dependencies */
import { isFunction } from 'lodash';

/* Internal dependencies */
import store from '../store';
import { setActivePanel } from '../store/actions';
import { applyFilters } from '@wordpress/hooks';

const sidebars = {};

/**
* Registers a sidebar with the editor.
*
* A button will be shown in the settings menu to open the sidebar. The sidebar
* can be manually opened by calling the `activateSidebar` function.
*
* @param {string} name The name of the sidebar. Should be in
* `[plugin]/[sidebar]` format.
* @param {Object} settings The settings for this sidebar.
* @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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@atimmer atimmer Jan 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
export function registerSidebar( name, settings ) {
settings = {
name,
...settings,
};

if ( typeof name !== 'string' ) {
console.error(
'Sidebar names must be strings.'
);
return;
}
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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing period

);
return;
}
if ( ! settings || ! isFunction( settings.render ) ) {
console.error(
'The "render" property must be specified and must be a valid function.'
);
return;
}
if ( sidebars[ name ] ) {
console.error(
'Sidebar "' + name + '" is already registered.'
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing return

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be intentional to overwrite a plugin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if ( ! ( 'title' in settings ) || settings.title === '' ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

console.error(
'The sidebar "' + name + '" must have a title.'
);
return;
}
if ( typeof settings.title !== 'string' ) {
console.error(
'Sidebar titles must be strings.'
);
return;
}

settings = applyFilters( 'editor.registerSidebar', settings, name );

return sidebars[ name ] = settings;
}

/**
* Retrieves the sidebar settings object.
*
* @param {string} name The name of the sidebar to retrieve.
*
* @returns {false|Object} The settings object of the sidebar. Or false if the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* sidebar doesn't exist.
*/
export function getSidebar( name ) {
if ( ! sidebars.hasOwnProperty( name ) ) {
return false;
}

return sidebars[ name ];
}

/**
* Retrieves all sidebars that are registered.
*
* @returns {Object[]} Registered sidebars.
*/
export function getSidebars() {
return sidebars;
}

/**
* Activates the gives sidebar.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the given sidebar

*
* @param {string} name The name of the sidebar to activate.
*/
export function activateSidebar( name ) {
store.dispatch( setActivePanel( name ) );
}
3 changes: 3 additions & 0 deletions editor/edit-post/header/ellipsis-menu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { IconButton, Dropdown } from '@wordpress/components';
import './style.scss';
import ModeSwitcher from '../mode-switcher';
import FixedToolbarToggle from '../fixed-toolbar-toggle';
import Plugins from '../plugins';

const element = (
<Dropdown
Expand All @@ -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 } />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks odd when no plugins exist:

no plugins

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

</div>
) }
/>
Expand Down
89 changes: 89 additions & 0 deletions editor/edit-post/header/plugins/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/**
* External dependencies
*/
import { map } from 'lodash';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { getActivePanel, isEditorSidebarOpened } from '../../../store/selectors';
import { connect } from 'react-redux';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved under "External dependencies"

import { toggleSidebar } from '../../../store/actions';

/**
* Renders a list of plugins that will activate different UI elements.
*
* @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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param {boolean} props.isSidebarOpened Whether the sidebar is currently open.
* @param {Function} props.onToggleSidebar Function to call when the sidebar
* should be toggled.
*
* @returns {Object} The rendered list of menu items.
*/
function Plugins( { activePanel, onSwitch, isSidebarOpened, onToggleSidebar } ) {
const sidebars = getSidebars();

// This makes sure no check mark is before a plugin if the sidebar is closed.
if ( ! isSidebarOpened ) {
activePanel = '';
}

/**
* Handles the user clicking on one of the plugins in the menu
*
* @param {string} panelToActivate The sidebar panel to activate.
*/
function onSelect( panelToActivate ) {
onSwitch( panelToActivate );

if ( ! isSidebarOpened ) {
onToggleSidebar();
}
}

const plugins = map( sidebars, ( sidebar ) => {
return {
value: sidebar.name,
label: sidebar.title,
};
} );

return (
<MenuItemsGroup
label={ __( 'Plugins' ) }
choices={ plugins }
value={ activePanel }
onSelect={ onSelect }
/>
);
}

export default connect(
( state ) => {
return {
activePanel: getActivePanel( state ),
isSidebarOpened: isEditorSidebarOpened( state ),
};
},
( dispatch, ownProps ) => {
return {
onSwitch: ( value ) => {
activateSidebar( value );
ownProps.onToggle( value );
},
onToggleSidebar: () => {
dispatch( toggleSidebar() );
},
};
}
)( Plugins );
65 changes: 61 additions & 4 deletions editor/edit-post/sidebar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { connect } from 'react-redux';
/**
* WordPress Dependencies
*/
import { __ } from '@wordpress/i18n';
import { withFocusReturn } from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';
import { withFocusReturn, Panel, PanelBody } from '@wordpress/components';

/**
* Internal Dependencies
Expand All @@ -16,10 +16,68 @@ import './style.scss';
import PostSettings from './post-settings';
import BlockInspectorPanel from './block-inspector-panel';
import Header from './header';
import { getSidebar } from '../../api/sidebar';

import { getActivePanel } from '../../store/selectors';

/**
* Returns the sidebar that should be rendered in the sidebar registered by
* plugins.
*
* @param {string} panel The currently active panel.
*
* @returns {Object} The React element to render as a panel.
*/
function getPluginSidebar( panel ) {
const pluginSidebar = getSidebar( panel );

if ( ! pluginSidebar ) {
return () => {
return <Panel>
<PanelBody>
{ sprintf( __( 'No matching plugin sidebar found for panel "%s"' ), panel ) }
</PanelBody>
</Panel>;
};
}

return pluginSidebar.render;
}

/**
* Returns the panel that should be rendered in the sidebar.
*
* @param {string} panel The currently active panel.
*
* @returns {Object} The React element to render as a panel.
*/
function getPanel( panel ) {
switch ( panel ) {
case 'document':
return PostSettings;

case 'block':
return BlockInspectorPanel;

default:
return getPluginSidebar( panel );
}
}

/**
* Renders a sidebar with the relevant panel.
*
* @param {string} panel The currently active panel.
*
* @returns {Object} The rendered sidebar.
*/
const Sidebar = ( { panel } ) => {
const ActivePanel = getPanel( panel );

const props = {
panel,
};

return (
<div
className="editor-sidebar"
Expand All @@ -28,8 +86,7 @@ const Sidebar = ( { panel } ) => {
tabIndex="-1"
>
<Header />
{ panel === 'document' && <PostSettings /> }
{ panel === 'block' && <BlockInspectorPanel /> }
<ActivePanel { ...props } />
</div>
);
};
Expand Down
1 change: 1 addition & 0 deletions editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { EditorProvider, ErrorBoundary } from './components';
import { initializeMetaBoxState } from './store/actions';

export * from './components';
export * from './api';
import store from './store'; // Registers the state tree

// Configure moment globally
Expand Down