Edit Post: Refactor More (Ellipsis) Menu by gziolo · Pull Request #5161 · WordPress/gutenberg

gziolo · 2018-02-20T11:58:13Z

Description

This PR adds a couple of refactorings to make More Menu extensible:

Renames EllipsisMenu to MoreMenu to align with what was discussed on Slack some time ago.
Removes div separators for the groups in the menu and replaces them with more flexible plain CSS solution.
Moves CopyContentButton and hooks from editor folder to editPost folder.
Makes all groups of menu items extensible by exposing filters that follow the same pattern: editPost.MoreMenu.${ groupName ).
Extracts MenuItemsChoice component to be able to pass it down to the menu group.

How to test?

Make sure all existing menu items display and work as before.

You can dynamically add new menu items by executing the following code:

wp.hooks.addFilter( 'editPost.MoreMenu.editor', 'core/edit-post/editor-callback', ( items ) => [ ...items, 'Editor item' ] );
wp.hooks.addFilter( 'editPost.MoreMenu.settings', 'core/edit-post/editor-callback', ( items ) => [ ...items, 'Settings item' ] );
wp.hooks.addFilter( 'editPost.MoreMenu.tools', 'core/edit-post/editor-callback', ( items ) => [ ...items, 'Tools item' ] );

You might need to reopen More Menu to see the changes applied. We can fix that in the follow-up PR if we decide to have it dynamically re-render when new filters gets added or removed. It can be achieved by extracting most of the logic or even updating withFilters HOC: https://github.com/WordPress/gutenberg/blob/master/components/higher-order/with-filters/index.js#L26.

Screenshots (jpeg or gifs if applicable):

More Menu with filters applied:

In the screenshot I added plain text, that’s why it doesn’t look perfect. We would rather expose a few components to consume there to make sure it looks properly.

Checklist:

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

youknowriad · 2018-02-20T12:15:22Z

edit-post/components/header/copy-content-menu-item/index.js

Maybe we should perform this call only when clicking on the button. Serializing blocks can harm performance.

Would you like to call it directly using wp.data.select or expose it as a function here?
@mcsf what's your take on this?

On a second thought, maybe it's not that severe because it's only shown when you open the dropdown menu, it's probably run only once because you don't edit content when this menu is open.

Right, it doesn't render when you edit your post. It only renders once when you open the dropdown.

Actually, ClipboardButton is flexible and accepts a function for text, so the following works:

diff --git a/edit-post/components/header/copy-content-menu-item/index.js b/edit-post/components/header/copy-content-menu-item/index.js index 88561ae1..a55688ad 100644 --- a/edit-post/components/header/copy-content-menu-item/index.js +++ b/edit-post/components/header/copy-content-menu-item/index.js @@ -2,11 +2,13 @@ * WordPress dependencies */ import { ClipboardButton, withState } from '@wordpress/components'; -import { compose } from '@wordpress/element'; -import { query } from '@wordpress/data'; +import { select } from '@wordpress/data'; import { __ } from '@wordpress/i18n'; -function CopyContentMenuItem( { editedPostContent, hasCopied, setState } ) { +const editedPostContent = () => + select( 'core/editor' ).getEditedPostAttribute( 'content' ); + +function CopyContentMenuItem( { hasCopied, setState } ) { return ( <ClipboardButton text={ editedPostContent } @@ -21,9 +23,6 @@ function CopyContentMenuItem( { editedPostContent, hasCopied, setState } ) { ); } -export default compose( - query( ( select ) => ( { - editedPostContent: select( 'core/editor' ).getEditedPostAttribute( 'content' ), - } ) ), - withState( { hasCopied: false } ) +export default withState( + { hasCopied: false } )( CopyContentMenuItem );

Not sure which is better. Probably can stay as it is.

jasmussen · 2018-02-20T13:11:45Z

Nice, this seems to work great! 👍 👍

mcsf

Like!

mcsf · 2018-02-20T15:16:56Z

edit-post/components/header/copy-content-menu-item/index.js

Actually, ClipboardButton is flexible and accepts a function for text, so the following works:

diff --git a/edit-post/components/header/copy-content-menu-item/index.js b/edit-post/components/header/copy-content-menu-item/index.js index 88561ae1..a55688ad 100644 --- a/edit-post/components/header/copy-content-menu-item/index.js +++ b/edit-post/components/header/copy-content-menu-item/index.js @@ -2,11 +2,13 @@ * WordPress dependencies */ import { ClipboardButton, withState } from '@wordpress/components'; -import { compose } from '@wordpress/element'; -import { query } from '@wordpress/data'; +import { select } from '@wordpress/data'; import { __ } from '@wordpress/i18n'; -function CopyContentMenuItem( { editedPostContent, hasCopied, setState } ) { +const editedPostContent = () => + select( 'core/editor' ).getEditedPostAttribute( 'content' ); + +function CopyContentMenuItem( { hasCopied, setState } ) { return ( <ClipboardButton text={ editedPostContent } @@ -21,9 +23,6 @@ function CopyContentMenuItem( { editedPostContent, hasCopied, setState } ) { ); } -export default compose( - query( ( select ) => ( { - editedPostContent: select( 'core/editor' ).getEditedPostAttribute( 'content' ), - } ) ), - withState( { hasCopied: false } ) +export default withState( + { hasCopied: false } )( CopyContentMenuItem );

mcsf · 2018-02-20T15:34:07Z

edit-post/hooks/more-menu/index.js

+/**
+ * Internal dependencies
+ */
+import CopyContentMenuItem from '../../components/header/copy-content-menu-item';


You're hooking CopyContentMenuItem into Gutenberg but keeping its definition outside edit-post/hooks. That's interesting; could you explain why?

I wasn't sure where to put it. I guess you are right that it should stay as a subfolder inside edit-post/hooks/ :)

aduth · 2018-02-23T05:07:47Z

components/menu-items/style.scss

@@ -1,9 +1,9 @@
-.components-choice-menu {
+.components-menu-items-group {


This should be .components-menu-items__group

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#css

Opened #5218 to address it.

aduth · 2018-02-23T05:09:17Z

edit-post/components/header/more-menu/index.js

-import EditorActions from '../editor-actions';

-const element = (
+const MoreMenu = () => (


Did this need to be changed to a function?

See also an explanation of this type of optimization: https://babeljs.io/docs/plugins/transform-react-constant-elements/

That's interesting. I haven't seen it before in the React docs.

This transform should be enabled only in production (e.g., just before minifying your code) because although it improves runtime performance, it makes warning messages more cryptic.

Can we enable the transform instead and leave all the low-level optimizations to Babel?

aduth · 2018-09-19T17:33:07Z

edit-post/components/header/more-menu/index.js

-				<EditorActions />
+				<MenuItemsGroup
+					label={ __( 'Tools' ) }
+					filterName="editPost.MoreMenu.tools"


Conceptually-speaking, why do we need two things - plugin slots and filterable menus - to express the same idea of "add to this menu" ? As of current master, we have both in this menu, and it's not clear why I would use one or the other, or why one can't be satisfied by the other.

gutenberg/edit-post/components/header/more-menu/index.js

Lines 36 to 40 in f175813

<PluginMoreMenuGroup.Slot fillProps={ { onClose } } />

<MenuGroup

label={ __( 'Tools' ) }

filterName="editPost.MoreMenu.tools"

>

My guess is filterable menus were some of the earlier experiments, partly motivated by the emergence of @wordpress/hooks. They also felt cheaper (perhaps deceptively simple?). I'd say by now slots have established themselves as the standard extension method, though.

cc @gziolo I think he was talking about removing those at some point?

Yes, we should remove those filters and replace the only occurrence of hook based extension with slot and fill. As @mcsf already pointed out, this was an early exploration which turned out to be suboptimal from developer’s standpoint.

I had it planned to refactor on my todos list but it was never a priority. I will take care of it this week to avoid confusion.

gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] UI Components Impacts or related to the UI component system labels Feb 20, 2018

gziolo self-assigned this Feb 20, 2018

gziolo requested review from aduth, jasmussen, mcsf and youknowriad February 20, 2018 11:58

gziolo changed the title ~~Edit Post: Refactor Ellipsis Menu~~ Edit Post: Refactor More (Ellipsis) Menu Feb 20, 2018

youknowriad reviewed Feb 20, 2018

View reviewed changes

gziolo force-pushed the update/menu-items-group branch from e16d4e0 to 4a5fb14 Compare February 20, 2018 12:37

jasmussen mentioned this pull request Feb 20, 2018

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

Closed

mcsf reviewed Feb 20, 2018

View reviewed changes

gziolo added 10 commits February 21, 2018 10:05

Edit Post: Move copy content to edit-post folder

92a52de

Components: Extract MenuItemsChoice component from MenuItemsGroup

01ecb21

EditPost: Refactor hooks for more menu component

705d783

Post edit: Rename Ellipsis Menu component to More Menu

e3df2c9

Edit post: Add the menu group separator using CSS styles

02fe29b

Components: Make MenuItemsGroup components extensible

235ef3a

Components: Stop rendering MenuItemsGroup when no menu items provided

3a1701f

Edit post: Fix spacing issues in more menu styles

98c64c5

Tests: Update e2e tests to use the updated more menu class

462482d

Edit post: Move CopyContentMenuItem to hooks subfolder

28e3031

gziolo force-pushed the update/menu-items-group branch from 5748678 to 28e3031 Compare February 21, 2018 09:06

mcsf approved these changes Feb 21, 2018

View reviewed changes

gziolo merged commit 38be3c9 into master Feb 22, 2018

gziolo deleted the update/menu-items-group branch February 22, 2018 06:55

gziolo mentioned this pull request Feb 22, 2018

Add More Menu Item Api #4484

Closed

3 tasks

aduth reviewed Feb 23, 2018

View reviewed changes

gziolo mentioned this pull request Feb 23, 2018

Edit Post: Refactor class names in More Menu component #5218

Merged

3 tasks

igorschoester mentioned this pull request Feb 27, 2018

Try gutenberg more menu plugin filter Yoast/wordpress-seo#9066

Closed

gziolo mentioned this pull request Mar 13, 2018

Jetpack Publicize missing from side panels #3264

Closed

aduth reviewed Sep 19, 2018

View reviewed changes

		@@ -1,9 +1,9 @@
		.components-choice-menu {
		.components-menu-items-group {

	<PluginMoreMenuGroup.Slot fillProps={ { onClose } } />
	<MenuGroup
	label={ __( 'Tools' ) }
	filterName="editPost.MoreMenu.tools"
	>

Conversation

gziolo commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test?

Screenshots (jpeg or gifs if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmussen commented Feb 20, 2018

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gziolo commented Feb 20, 2018 •

edited

Loading

gziolo Feb 20, 2018 •

edited

Loading

gziolo Feb 23, 2018 •

edited

Loading