Reuse and unify post and page actions, accross the different use cases. #59849

jorgefilipecosta · 2024-03-13T21:34:02Z

This PR implements the post and page actions in a single place (editor packages):
And reuses the actions in the following scenarios:

Dataviews item action and bulk action.
Details view actions (left sidebar).
Edit site page inspector.
Edit post inspector.

The edit post inspector in trunk does not contain a "summary view at the top with the actions" this PR adds it to make things consistent.

Screenshots

Testing Instructions for Keyboard

Verified in all the cases where the page actions are used that they work as expected.

github-actions · 2024-03-13T21:34:15Z

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
Labels found: [Type] Enhancement, [Type] Code Quality.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

github-actions · 2024-03-13T21:34:20Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: mcsf <mcsf@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ntsekouras · 2024-03-14T08:18:42Z

That's a great start, thanks! We can start testing/reviewing here, but at first sight I was wondering if we could split this to some chunks to make things easier. There are quite a few changes here and we might miss some things and also this PR might take a bit more time to land..

What do you think? For example PostSidebarCard would land super quick 😄

youknowriad · 2024-03-14T09:07:43Z

packages/edit-post/src/components/sidebar/post-initial-panel/index.js

+	postRevisionsAction,
+} = postManagementActions;
+
+function PostActions( { postType, postId } ) {


What is specific about edit-post that is forcing us to override the default "editor" PostActions?

When we execute an action, it may require us to navigate to a different page. For example, when we delete a post while editing it, we need to navigate to a completely different page using "window.location.href". This is a unique requirement on the edit-post. We are not replacing the action, but rather, we are adding specific callbacks to the onPerform function when needed.

But it's almost always the same kind of override. So rather than all of this complex code, some simple onNavigateToList/onNavigateToPost props or something like that could work.

But it's almost always the same kind of override. So rather than all of this complex code, some simple onNavigateToList/onNavigateToPost props or something like that could work.

That would work. Thank you for the suggestion @youknowriad 👍 Another possibility is a a generic usePostActions( onPerform ) with a global onPerform( actionId, items) that receives the actionId and the items where the action was executed. And then the usage would be very simple:

usePostActions( onPerform( actionId, items)=> { switch(actionId) case: moveToTrass case: permanentlyDelete: onNavigateToList(); break: case: editPost: navigateToPost } );

Let me know which API would you prefer a onNavigateToList and a onNavigateToPost or a global onPerform.

Maybe the generic onPerform is slightly better.

onPerform is not very obvious. Maybe onNavigateAfterAction? onAfterAction? onActionPerformed?

I wonder whether that API is too powerful, though. Concerns:

For the reader, how obvious is it that this prop is meant to strictly set whether and where to route after the action is completed?

Is there a clearer and stricter alternative?

Does this introduce indirection, in the sense that the reader needs to juggle two distinct components to understand the whole cascade of effects?

(This is not a strong opinion, just caution.)

Good points. The onPerform may be used for other things besides navigation e.g: close a menu after the action is performed etc. And we already had onPerform for actions before:

gutenberg/packages/edit-site/src/components/actions/index.js

Lines 150 to 153 in 9c0a5ac

if ( onPerform ) {

onPerform();

}

closeModal();

But not all of the actions had it.

I think I will go for onActionPerformed, it more clear on what it does and what is expected when compared with just onPerform.

youknowriad · 2024-03-14T09:09:58Z

packages/edit-site/src/components/page-pages/index.js

+				postId: post.id,
+				postType: post.type,
+				canvas: 'edit',
+			} );


I believe we have some "commands" that either navigate using the url or use history depending on whether we detect that we're in the site editor. Can't we do the same thing within the shared action and avoid overriding it entirely?

The action itself does not determine whether navigation is necessary or not. For instance, on data views (such as the posts list), the action is used as it is, without requiring any navigation. It is the specific location where the action is utilized that determines whether performing an action has a side effect, such as navigation.

The commands always navigate to the site editor, even if the post was in the post editor:

const targetUrl = addQueryArgs( 'site-editor.php', args ); if ( isSiteEditor ) { history.push( args ); } else { document.location = targetUrl; }

When we are in the post editor and a post is deleted we want to go back to the "old post" list "edit.php?post_type=page" at least for now. We may also need to go to "edit.php?post_type=post" or to "edit.php?post_type=custom_post_type" so we don't want to go to the site editor every time like in commands.

We could have in the editor something like

usePostActions(withNavigation =true/false)

Where
usePostActions(false) is used on dataviews.

usePostActions(true) is used on post editor, site editor details and site editor inspector.

When it is true we would have some logic to check if we are in the site editor isSiteEditor(). If yes we navigate in a way if not we navigate in another way. But that approach seems wrong to me it is editor package that is implementing the specific logic of post-edit, and site-edit while to me it seems it should be the other way around.

youknowriad · 2024-03-14T09:11:31Z

packages/edit-site/src/components/post-actions/index.js

+	return actions;
+}
+
+export default function PostActions( { postType, postId } ) {


Why are we overriding the default PostActions here, what's specific to the site editor?

As referred above what is specific in this case is that for certain actions we need to navigate using history.push, while in other cases we should not navigate at all or we should navigate to a different page.

jorgefilipecosta · 2024-03-14T11:15:38Z

That's a great start, thanks! We can start testing/reviewing here, but at first sight I was wondering if we could split this to some chunks to make things easier. There are quite a few changes here and we might miss some things and also this PR might take a bit more time to land..

What do you think? For example PostSidebarCard would land super quick 😄

Hi @ntsekouras, yes the idea is to merge things in chunks but having a working end result, allows us to have a better idea of the direction.

A PR for PostSidebarCard is available at #59870.
The next PR is one that just moves the actions to WordPress editor without using the actions in new places.
And the final PR is the usage of the actions across the new places.

mcsf · 2024-03-15T16:22:13Z

packages/editor/src/components/post-actions/utility-components.js

+	);
+}
+
+// Copied as is from packages/dataviews/src/utils.js


Sometimes just copying + duplicating a small thing is nice, rather than defaulting to DRY. I think it's underrated, even. But in this case I would either:

Duplicate directly to where it will be used, i.e. editor/src/components/post-actions/index.js and don't export as a util.

Make the decision to upgrade WithDropDownMenuSeparators to something reusable, in which case it belongs in @wordpress/components alongside DropdownMenuSeparator. Although, honestly, I'm more in favour of the first option for now.

Hi @mcsf the reason for "post-actions/utility-components.js" was to have a file where everything inside it is duplicated code. But keeping it where it used also works and we can have a "start duplicated components" and an "end duplicated components" mark.

mcsf · 2024-03-15T16:24:10Z

packages/editor/src/components/post-actions/utility-components.js

+// Copied as is from packages/dataviews/src/item-actions.js
+export function ActionsDropdownMenuGroup( { actions, item } ) {


Same observation as for WithDropDownMenuSeparators.

packages/editor/src/components/post-actions/utility-components.js

mcsf · 2024-03-15T16:31:16Z

packages/edit-post/src/components/sidebar/post-initial-panel/index.js

+	postRevisionsAction,
+} = postManagementActions;
+
+function PostActions( { postType, postId } ) {


onPerform is not very obvious. Maybe onNavigateAfterAction? onAfterAction? onActionPerformed?

I wonder whether that API is too powerful, though. Concerns:

For the reader, how obvious is it that this prop is meant to strictly set whether and where to route after the action is completed?

Is there a clearer and stricter alternative?

Does this introduce indirection, in the sense that the reader needs to juggle two distinct components to understand the whole cascade of effects?

(This is not a strong opinion, just caution.)

mcsf · 2024-03-15T16:32:54Z

packages/edit-post/src/components/sidebar/post-initial-panel/index.js

+							if ( props.onPerform ) {
+								props.onPerform();
+							}


Just for fun:

Suggested change

if ( props.onPerform ) {

props.onPerform();

}

props.onPerform?.();

Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>

jorgefilipecosta added 9 commits March 13, 2024 11:20

Add: Reusable post actions.

9b24862

in progress

431b77b

reuse actions on details view

ae011ed

working across different actions uses.

5340396

in progress

d92a6b1

fix onPerform

fe078b0

use actions on data views too

1464197

code quality enhacements

32a9320

lint fixes

6116b91

jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality labels Mar 13, 2024

jorgefilipecosta requested review from youknowriad and ntsekouras March 13, 2024 21:34

ntsekouras requested a review from mcsf March 14, 2024 08:18

ntsekouras mentioned this pull request Mar 14, 2024

Tracking issue: Normalize details view with inspector controls in site editor #59689

Closed

7 tasks

youknowriad reviewed Mar 14, 2024

View reviewed changes

hasEdit

3248418

jorgefilipecosta mentioned this pull request Mar 14, 2024

Add: PostCardPanel component. #59870

Merged

mcsf reviewed Mar 15, 2024

View reviewed changes

Update packages/editor/src/components/post-actions/utility-components.js

2f87575

Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>

jorgefilipecosta mentioned this pull request Mar 21, 2024

Update: Move post actions to the editor package. #60092

Merged

jorgefilipecosta closed this Apr 4, 2024

jorgefilipecosta mentioned this pull request Apr 4, 2024

Reuse and unify post and page actions, accross the different use cases. #60486

Merged

youknowriad deleted the update/reusable-post-actions branch April 5, 2024 05:29

jorgefilipecosta mentioned this pull request Apr 5, 2024

Site Editor: Add rename and trash actions to page panel #60232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse and unify post and page actions, accross the different use cases. #59849

Reuse and unify post and page actions, accross the different use cases. #59849

jorgefilipecosta commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024 •

edited

Loading

ntsekouras commented Mar 14, 2024

youknowriad Mar 14, 2024

jorgefilipecosta Mar 14, 2024

youknowriad Mar 14, 2024

jorgefilipecosta Mar 14, 2024

youknowriad Mar 14, 2024

mcsf Mar 15, 2024

jorgefilipecosta Mar 21, 2024

youknowriad Mar 14, 2024

jorgefilipecosta Mar 14, 2024

youknowriad Mar 14, 2024

jorgefilipecosta Mar 14, 2024

jorgefilipecosta commented Mar 14, 2024

mcsf Mar 15, 2024

jorgefilipecosta Mar 21, 2024

mcsf Mar 15, 2024

mcsf Mar 15, 2024

mcsf Mar 15, 2024

		// Copied as is from packages/dataviews/src/item-actions.js
		export function ActionsDropdownMenuGroup( { actions, item } ) {

Reuse and unify post and page actions, accross the different use cases. #59849

Reuse and unify post and page actions, accross the different use cases. #59849

Conversation

jorgefilipecosta commented Mar 13, 2024

Screenshots

Testing Instructions for Keyboard

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024 • edited Loading

ntsekouras commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2024 •

edited

Loading