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 More Menu Item Api #4484

Closed
wants to merge 75 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
d971eb5
Copied mfolkeseth's implementation of the ellipsis menu api from http…
Jan 15, 2018
cec1e66
implemented callback to api
Jan 15, 2018
54e979d
Minor refactoring
Jan 15, 2018
230c82a
Hide divider in ellipsis menu when no plugins are present
Jan 15, 2018
2a2224f
Close menu on plugin menu item click
Jan 15, 2018
011544a
Merge branch 'master' into add/api-add-ellipsis-menu-item
xyfi Jan 15, 2018
8ca0ac2
minor JSDoc and codestyle fixes
Jan 15, 2018
0b16b6c
Merge branch 'add/api-add-ellipsis-menu-item' of https://github.com/Y…
Jan 15, 2018
399d526
Eslint fixes
Jan 15, 2018
2abf2e5
Merge branch 'master' into add/api-add-ellipsis-menu-item
Jan 16, 2018
b6e9d92
Moved Plugins menu items in ellipsis menu above Editor Actions
Jan 16, 2018
8d0de21
Refactored Plugins component to not use MenuItemsToggle component
Jan 16, 2018
050e377
Added .idea to .gitignore
Jan 19, 2018
7fa0a42
Merge branch 'master' into add/api-add-ellipsis-menu-item
Jan 19, 2018
b68614f
Basic icon functionality
Jan 22, 2018
0e03a43
restrain SVGs
Jan 22, 2018
f7e4ac4
Css improvements
Jan 23, 2018
a9f7c8f
Introduced MenuItemsSeparator component
Jan 23, 2018
95c816d
Applied CR feedback
Jan 23, 2018
438015d
removed redudant css
Jan 23, 2018
b762428
Changed api to registerEditorMenuItem to be more agnostic
Jan 31, 2018
31d50a8
Fixed error in plugins/index.js
Jan 31, 2018
1fdf22c
Merge master
Jan 31, 2018
8997346
Merge branch 'master' into add/api-add-ellipsis-menu-item
Feb 7, 2018
f61a32b
Fixed eslint errors
Feb 7, 2018
8bcb6fb
Separated plugin and editor menu item logic
Feb 9, 2018
ffc6038
Fixed eslint error
Feb 9, 2018
9779e93
Reuse validatePluginId function in editor-menu-item.js
Feb 9, 2018
2ae47c6
Replaced concatenated string with template string in editor-menu-item.js
Feb 9, 2018
4e4dee1
Renamed plugin id to target
Feb 12, 2018
07b98e5
Merge branch 'master' into add/api-add-ellipsis-menu-item
Feb 19, 2018
daa0605
Added check for existing registered plugin to plugin-core
Feb 19, 2018
b1ab904
Added tests for plugin-core.js
Feb 19, 2018
74c12d8
Initial test setup for editor-menu-item.js
Feb 19, 2018
4178f8c
Implemented test for editor-menu-item.js
Feb 19, 2018
4a84ec2
Merged master
Feb 21, 2018
27675de
Removed IDE specific .gitignore line
Feb 22, 2018
c1c9a9a
Merged master
Feb 22, 2018
31a3815
Removed plugins-core
Feb 27, 2018
37590b2
Renamed editor- and ellipsis menu items to more menu items
Feb 27, 2018
020e191
Merge branch 'master' into add/api-add-ellipsis-menu-item
xyfi Feb 27, 2018
d19a282
Removed unnecessary comment
Feb 27, 2018
e29e15e
Merge branch 'add/api-add-ellipsis-menu-item' of https://github.com/Y…
Feb 27, 2018
cceb1ad
Show active lugin button in more menu
Feb 27, 2018
7496fd3
Merge branch 'master' into add/api-add-ellipsis-menu-item
Feb 27, 2018
b2f342a
Improved logic to detect whther a plugin is active in More Menu
Feb 27, 2018
79fe168
Show check-mark instead of icon when plugin is active in more menu
Feb 28, 2018
2579564
Menu item now shows 'yes' icon when plugin is active and plugin has n…
Feb 28, 2018
54f2417
Fixed invalid HTML by using span instead of div inside button to cont…
Feb 28, 2018
dcab4c1
Removed unsed css due to refactoring to MenuItemsGroup in plugins menu
Feb 28, 2018
1c01707
Moved filter before validation
Feb 28, 2018
1803d1a
Removed unnecessary import statement
Feb 28, 2018
c0be60b
Removed unnecessary import statement
Feb 28, 2018
da572d8
Moved validatePlugin function to root utils folder
Feb 28, 2018
b95e43d
Don't expose getMoreMenuItems
Feb 28, 2018
4d544ec
Added test to test prevention of duplicate menu item ids
Feb 28, 2018
9acbe10
Renamed registerMoreMenuItem to __experimentalRegisterMoreMenuApi
Feb 28, 2018
e55bdcc
Changed __experimentalRegisterMoreMenuItem to experimental__registerM…
Feb 28, 2018
2679098
Merge branch 'master' into add/api-add-ellipsis-menu-item
Feb 28, 2018
e70c88d
Renamed validatePlugin to validateNamespacedId, and improved document…
Feb 28, 2018
ebafa1c
Various minor codestyle changes
Feb 28, 2018
d8afc30
Use validateNamespaceId from @wordpress/utils instead of relative path
Feb 28, 2018
2ff0b8e
Switched to isString from lodash instead of utils in more-menu-item.js
Feb 28, 2018
e71388b
Fixed tests
Feb 28, 2018
5ece508
Moved validateNamespacedId imports to Wordpress Dependencies in sever…
Feb 28, 2018
46642fa
Various JSDoc changes
Feb 28, 2018
fdef65d
Removed unnecessary indentation in header/plugins/index.js
Feb 28, 2018
0a57601
Switched from connect to compose and withSelect in header/plugins/ind…
Feb 28, 2018
8e97836
Use wordpress dependency @wordpress/i18n instead of global wp.i18n in
Mar 1, 2018
b9631b7
Updated test desciption to better reflect the tested function's intent
Mar 1, 2018
3910b3f
Changed registerMoreMenuItem JSDoc
Mar 1, 2018
8ceca16
Aligned parameters in utils/plugins.js
Mar 1, 2018
a48baf8
Updated classnames to meet recommended standards in edit-post's plugi…
Mar 1, 2018
ff5d2df
Removed some old code that wasn't relevant anymore
Mar 1, 2018
dca2e0e
Changed experimental__registerMoreMenuApi to __experimentalRegisterMo…
Mar 1, 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
12 changes: 2 additions & 10 deletions blocks/api/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { get, isFunction, some } from 'lodash';
* WordPress dependencies
*/
import { applyFilters } from '@wordpress/hooks';
import { validateNamespacedId } from '@wordpress/utils';

/**
* Internal dependencies
Expand Down Expand Up @@ -94,16 +95,7 @@ export function registerBlockType( name, settings ) {
...settings,
};

if ( typeof name !== 'string' ) {
console.error(
'Block names must be strings.'
);
return;
}
if ( ! /^[a-z][a-z0-9-]*\/[a-z][a-z0-9-]*$/.test( name ) ) {
console.error(
'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block'
);
if ( ! validateNamespacedId( name, 'Block names' ) ) {
return;
}
if ( blocks[ name ] ) {
Expand Down
10 changes: 5 additions & 5 deletions blocks/api/test/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,31 +51,31 @@ describe( 'blocks', () => {

it( 'should reject blocks without a namespace', () => {
const block = registerBlockType( 'doing-it-wrong' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-id' );
expect( block ).toBeUndefined();
} );

it( 'should reject blocks with too many namespaces', () => {
const block = registerBlockType( 'doing/it/wrong' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-id' );
expect( block ).toBeUndefined();
} );

it( 'should reject blocks with invalid characters', () => {
const block = registerBlockType( 'still/_doing_it_wrong' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-id' );
expect( block ).toBeUndefined();
} );

it( 'should reject blocks with uppercase characters', () => {
const block = registerBlockType( 'Core/Paragraph' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-id' );
expect( block ).toBeUndefined();
} );

it( 'should reject blocks not starting with a letter', () => {
const block = registerBlockType( 'my-plugin/4-fancy-block', defaultBlockSettings );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block' );
expect( console ).toHaveErroredWith( 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-id' );
expect( block ).toBeUndefined();
} );

Expand Down
3 changes: 3 additions & 0 deletions edit-post/api/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
export {
registerMoreMenuItem as __experimentalRegisterMoreMenuItem,
} from './more-menu-item';
export {
registerSidebar,
activateSidebar,
Expand Down
91 changes: 91 additions & 0 deletions edit-post/api/more-menu-item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/* eslint no-console: [ 'error', { allow: [ 'error' ] } ] */

/**
Copy link
Member

Choose a reason for hiding this comment

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

Empty dependency set can be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* Wordpress dependencies
*/
import { applyFilters } from '@wordpress/hooks';
import { validateNamespacedId } from '@wordpress/utils';

/**
* Internal dependencies
*/
import { isString } from 'lodash';
import { activateSidebar } from './sidebar';

const menuItems = {};

/**
* Registers a plugin under the more menu.
*
* @param {string} menuItemId The unique identifier of the plugin. Should be in
* `[namespace]/[name]` format.
* @param {Object} settings The settings for this menu item.
* @param {string} settings.title The name to show in the settings menu.
* @param {func} settings.target The registered plugin that should be activated.
* @param {ReactElement} [settings.icon] SVG React Element.
*
* @return {Object} The final sidebar settings object.
*/
export function registerMoreMenuItem( menuItemId, settings ) {
settings = {
menuItemId,
...settings,
};

settings = applyFilters( 'editor.registerMoreMenuItem', settings, menuItemId );
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 prefixed editPost., not editor., given the file location.

Copy link
Member

Choose a reason for hiding this comment

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

Is passing menuItemId as an argument necessary if it's available as a property of the settings object?


if ( ! validateNamespacedId( menuItemId ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

All this common validation makes me wonder at what point we should consider reaching for a full-fledge solution built for this type of thing, such as Yup (Joi) or JSON Schema.

return null;
Copy link
Member

Choose a reason for hiding this comment

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

Is the discrepancy between this registration's returning of null and block's undefined return; intentional? If it's an issue that block's registration should return a non-undefined value, is there a plan to follow-through with a pull request to keep block registration consistent as well?

}
if ( menuItems[ menuItemId ] ) {
console.error(
`Menu item "${ menuItemId }" is already registered.`
);
}

if ( ! settings.title ) {
console.error(
`Menu item "${ menuItemId }" must have a title.`
);
return null;
}
if ( typeof settings.title !== 'string' ) {
console.error(
'Menu items title must be strings.'
);
return null;
}

if ( settings.icon && isString( settings.icon ) ) {
console.error(
'Menu item icon must be a react component'
);
return null;
}

if ( ! settings.target ) {
console.error(
`Menu item "${ menuItemId }" must have a target.`
);
return null;
}
if ( typeof settings.target !== 'string' ) {
console.error(
'Menu items target must be strings.'
);
return null;
}

settings.callback = activateSidebar.bind( null, settings.target );

return menuItems[ menuItemId ] = settings;
}

/**
* Retrieves all menu items that are registered.
*
* @return {Object} Registered menu items.
*/
export function getMoreMenuItems() {
return menuItems;
}
26 changes: 13 additions & 13 deletions edit-post/api/sidebar.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
/* eslint no-console: [ 'error', { allow: [ 'error' ] } ] */

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

/* Internal dependencies */
/**
* Wordpress dependencies
*/
import { applyFilters } from '@wordpress/hooks';
import { validateNamespacedId } from '@wordpress/utils';

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

const sidebars = {};

Expand All @@ -30,16 +39,7 @@ export function registerSidebar( name, settings ) {
...settings,
};

if ( typeof name !== 'string' ) {
console.error(
'Sidebar names must be strings.'
);
return null;
}
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.'
);
if ( ! validateNamespacedId( name ) ) {
return null;
}
if ( ! settings || ! isFunction( settings.render ) ) {
Expand Down
43 changes: 43 additions & 0 deletions edit-post/api/test/more-menu-item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
let registerMoreMenuItem, getMoreMenuItems, registerSidebar;
function requireAll() {
jest.resetModules();
const sidebar = require( '../sidebar' );
registerSidebar = sidebar.registerSidebar;
const moreMenuItem = require( '../more-menu-item' );
registerMoreMenuItem = moreMenuItem.registerMoreMenuItem;
getMoreMenuItems = moreMenuItem.getMoreMenuItems;
}

requireAll();

describe( 'registerMoreMenuItem', () => {
beforeEach( requireAll );
Copy link
Member

Choose a reason for hiding this comment

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

Every test suite is executed in isolation. There is only one test in this file, so it is totally fine to import all dependencies using import statements in the same way as you would do it in production code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added another test, because I do believe that if we would add more tests it's important that they are executed in a clean context.

Copy link
Member

Choose a reason for hiding this comment

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

Worth pointing out that resetting the Node require cache on every test case is not scalable, and its use should be limited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand, ideally I would like only one module to be reset, but I haven't found out how.


it( 'successfully registers a more menu item', () => {
registerSidebar( 'plugins/sidebar', {
title: 'Plugin Title',
render: () => 'Component',
} );
registerMoreMenuItem( 'gutenberg/plugin', {
title: 'Plugin',
target: 'gutenberg/plugin',
} );
expect( getMoreMenuItems()[ 'gutenberg/plugin' ] ).toBeDefined();
} );

it( 'throws an error when a more menu item with the same id already exists', () => {
registerSidebar( 'plugins/sidebar', {
title: 'Plugin Title',
render: () => 'Component',
} );
registerMoreMenuItem( 'gutenberg/plugin', {
title: 'Plugin',
target: 'gutenberg/plugin',
} );
registerMoreMenuItem( 'gutenberg/plugin', {
title: 'Plugin',
target: 'gutenberg/plugin',
} );
expect( console ).toHaveErrored();
} );
} );
2 changes: 2 additions & 0 deletions edit-post/components/header/more-menu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { IconButton, Dropdown, MenuItemsGroup } from '@wordpress/components';
import './style.scss';
import ModeSwitcher from '../mode-switcher';
import FixedToolbarToggle from '../fixed-toolbar-toggle';
import Plugins from '../plugins';

const MoreMenu = () => (
<Dropdown
Expand All @@ -27,6 +28,7 @@ const MoreMenu = () => (
<div className="edit-post-more-menu__content">
<ModeSwitcher onSelect={ onClose } />
<FixedToolbarToggle onToggle={ onClose } />
<Plugins onSelect={ onClose } />
<MenuItemsGroup
label={ __( 'Tools' ) }
filterName="editPost.MoreMenu.tools"
Expand Down
99 changes: 99 additions & 0 deletions edit-post/components/header/plugins/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/**
* External dependencies
*/
import { map, isEmpty } from 'lodash';
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { withInstanceId, IconButton, MenuItemsGroup } from '@wordpress/components';
import { withSelect } from '@wordpress/data';
import { compose } from '@wordpress/element';

/**
* Internal dependencies
*/
import './style.scss';
import { getMoreMenuItems } from '../../../api/more-menu-item';

/**
* Renders a list of plugins that will activate different UI elements.
*
* @param {Object} props The component props.
*
* @return {Object} The rendered list of menu items.
*/
function Plugins( props ) {
const ellipsisMenuItems = getMoreMenuItems();

if ( isEmpty( ellipsisMenuItems ) ) {
return null;
}

/**
* Handles the user clicking on one of the plugins in the menu
*
* @param {string} pluginId The plugin id.
*
* @return {void}
*/
function onSelect( pluginId ) {
props.onSelect();
ellipsisMenuItems[ pluginId ].callback();
}

return (
<MenuItemsGroup
label={ __( 'Plugins' ) }
filterName="editPost.MoreMenu.plugins" >
{ map( ellipsisMenuItems, menuItem => {
const pluginActive = menuItem.target === props.activePlugin;

let Icon = menuItem.icon ? (
<span className="edit-post-plugins__icon-container" >
{ menuItem.icon }
</span>
) : null;

if ( pluginActive ) {
Icon = 'yes';
}

const buttonClassName = classnames(
'edit-post-plugins__button',
{
'has-icon': Icon,
'is-active': pluginActive,
}
);

return (
<IconButton
key={ menuItem.menuItemId }
className={ buttonClassName }
icon={ Icon }
onClick={ () => onSelect( menuItem.menuItemId ) }>
{ menuItem.title }
</IconButton>
);
} ) }
</MenuItemsGroup>
);
}

export default compose( [
withSelect( select => {
const editPost = select( 'core/edit-post' );
const openedSidebar = editPost.getOpenedGeneralSidebar();
if ( openedSidebar !== 'plugin' ) {
return {};
}

return {
activePlugin: editPost.getActivePlugin(),
};
} ),
withInstanceId,
] )( Plugins );
32 changes: 32 additions & 0 deletions edit-post/components/header/plugins/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
.edit-post-plugins__button,
.edit-post-plugins__button.components-icon-button {
width: 100%;
padding: 8px 0;
padding-left: 25px;
text-align: left;
color: $dark-gray-500;

.dashicon {
margin-right: 5px;
}

&.has-icon {
padding-left: 0;
}

&:hover {
color: $black;
}
}

.edit-post-plugins__icon-container {
margin-right: 5px;
width: 20px;
height: 20px;
overflow: hidden;

svg {
Copy link
Member

Choose a reason for hiding this comment

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

How do we know there's an SVG ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment

width: 100%;
height: 100%;
}
}
Loading