Edit Post: Refactor More (Ellipsis) Menu

[gziolo]

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]

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

[gziolo]

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?

[youknowriad]

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.

[gziolo]

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

[mcsf]

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

[gziolo]

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

[jasmussen]

Nice, this seems to work great! 👍 👍

[mcsf]

Like!

[mcsf]

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]

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

[gziolo]

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

[gziolo]

Moved back

[aduth]

This should be .components-menu-items__group

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

[gziolo]

Updating

[gziolo]

Opened #5218 to address it.

[aduth]

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/

[gziolo]

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]

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.

[aduth]

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"
	>

[mcsf]

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.

[youknowriad]

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

[gziolo]

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.