-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update: Reuse and unify template actions. #60754
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,12 @@ import { | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { TEMPLATE_ORIGINS, TEMPLATE_POST_TYPE } from '../../store/constants'; | ||
import { | ||
TEMPLATE_ORIGINS, | ||
TEMPLATE_POST_TYPE, | ||
TEMPLATE_PART_POST_TYPE, | ||
PATTERN_POST_TYPE, | ||
} from '../../store/constants'; | ||
import { store as editorStore } from '../../store'; | ||
import { unlock } from '../../lock-unlock'; | ||
import isTemplateRevertable from '../../store/utils/is-template-revertable'; | ||
|
@@ -33,13 +38,19 @@ function getItemTitle( item ) { | |
return decodeEntities( item.title?.rendered || '' ); | ||
} | ||
|
||
const SITE_EDITING_POST_TYPES = [ | ||
TEMPLATE_POST_TYPE, | ||
TEMPLATE_PART_POST_TYPE, | ||
PATTERN_POST_TYPE, | ||
]; | ||
|
||
const trashPostAction = { | ||
id: 'move-to-trash', | ||
label: __( 'Move to Trash' ), | ||
isPrimary: true, | ||
icon: trash, | ||
isEligible( { status } ) { | ||
return status !== 'trash'; | ||
isEligible( { type, status } ) { | ||
return status !== 'trash' && ! SITE_EDITING_POST_TYPES.includes( type ); | ||
}, | ||
supportsBulk: true, | ||
hideModalHeader: true, | ||
|
@@ -182,8 +193,11 @@ function usePermanentlyDeletePostAction() { | |
id: 'permanently-delete', | ||
label: __( 'Permanently delete' ), | ||
supportsBulk: true, | ||
isEligible( { status } ) { | ||
return status === 'trash'; | ||
isEligible( { status, type } ) { | ||
return ( | ||
status === 'trash' && | ||
! SITE_EDITING_POST_TYPES.includes( type ) | ||
); | ||
}, | ||
async callback( posts, onActionPerformed ) { | ||
const promiseResult = await Promise.allSettled( | ||
|
@@ -292,8 +306,11 @@ function useRestorePostAction() { | |
isPrimary: true, | ||
icon: backup, | ||
supportsBulk: true, | ||
isEligible( { status } ) { | ||
return status === 'trash'; | ||
isEligible( { status, type } ) { | ||
return ( | ||
status === 'trash' && | ||
! SITE_EDITING_POST_TYPES.includes( type ) | ||
); | ||
}, | ||
async callback( posts, onActionPerformed ) { | ||
try { | ||
|
@@ -371,7 +388,10 @@ const viewPostAction = { | |
isPrimary: true, | ||
icon: external, | ||
isEligible( post ) { | ||
return post.status !== 'trash'; | ||
return ( | ||
post.status !== 'trash' && | ||
! SITE_EDITING_POST_TYPES.includes( post.type ) | ||
); | ||
}, | ||
callback( posts, onActionPerformed ) { | ||
const post = posts[ 0 ]; | ||
|
@@ -387,21 +407,25 @@ const editPostAction = { | |
label: __( 'Edit' ), | ||
isPrimary: true, | ||
icon: edit, | ||
isEligible( { status } ) { | ||
return status !== 'trash'; | ||
isEligible( { status, type } ) { | ||
return status !== 'trash' && ! SITE_EDITING_POST_TYPES.includes( type ); | ||
}, | ||
callback( posts, onActionPerformed ) { | ||
if ( onActionPerformed ) { | ||
onActionPerformed( posts ); | ||
} | ||
}, | ||
}; | ||
|
||
const postRevisionsAction = { | ||
id: 'view-post-revisions', | ||
label: __( 'View revisions' ), | ||
isPrimary: false, | ||
isEligible: ( post ) => { | ||
if ( post.status === 'trash' ) { | ||
if ( | ||
post.status === 'trash' || | ||
SITE_EDITING_POST_TYPES.includes( post.type ) | ||
) { | ||
return false; | ||
} | ||
const lastRevisionId = | ||
|
@@ -426,7 +450,10 @@ const renamePostAction = { | |
id: 'rename-post', | ||
label: __( 'Rename' ), | ||
isEligible( post ) { | ||
return post.status !== 'trash'; | ||
return ( | ||
post.status !== 'trash' && | ||
! SITE_EDITING_POST_TYPES.includes( post.type ) | ||
); | ||
}, | ||
RenderModal: ( { items, closeModal, onActionPerformed } ) => { | ||
const [ item ] = items; | ||
|
@@ -534,7 +561,7 @@ const resetTemplateAction = { | |
: sprintf( | ||
/* translators: The template/part's name. */ | ||
__( '"%s" reset.' ), | ||
decodeEntities( items[ 0 ].title.rendered ) | ||
getItemTitle( items[ 0 ] ) | ||
), | ||
{ | ||
type: 'snackbar', | ||
|
@@ -603,7 +630,11 @@ const resetTemplateAction = { | |
* @return {boolean} Whether the template is revertable. | ||
*/ | ||
function isTemplateRemovable( template ) { | ||
if ( ! template ) { | ||
if ( | ||
! template || | ||
( template.type !== TEMPLATE_POST_TYPE && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels weird to be inside a function called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch 😅 I will fix this. |
||
template.type !== TEMPLATE_PART_POST_TYPE ) | ||
) { | ||
return false; | ||
} | ||
|
||
|
@@ -636,9 +667,7 @@ const deleteTemplateAction = { | |
: sprintf( | ||
// translators: %s: The template or template part's titles | ||
__( 'Delete "%s"?' ), | ||
decodeEntities( | ||
templates?.[ 0 ]?.title?.rendered | ||
) | ||
getItemTitle( templates[ 0 ] ) | ||
) } | ||
</Text> | ||
<HStack justify="right"> | ||
|
@@ -679,7 +708,7 @@ const renameTemplateAction = { | |
}, | ||
RenderModal: ( { items: templates, closeModal, onActionPerformed } ) => { | ||
const template = templates[ 0 ]; | ||
const title = decodeEntities( template.title.rendered ); | ||
const title = getItemTitle( template ); | ||
const [ editedTitle, setEditedTitle ] = useState( title ); | ||
const { | ||
editEntityRecord, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,7 @@ import { moreVertical } from '@wordpress/icons'; | |
import { unlock } from '../../lock-unlock'; | ||
import { usePostActions } from './actions'; | ||
import { store as editorStore } from '../../store'; | ||
import { | ||
TEMPLATE_POST_TYPE, | ||
TEMPLATE_PART_POST_TYPE, | ||
PATTERN_POST_TYPE, | ||
} from '../../store/constants'; | ||
import { PATTERN_POST_TYPE } from '../../store/constants'; | ||
|
||
const { | ||
DropdownMenuV2: DropdownMenu, | ||
|
@@ -35,7 +31,10 @@ const POST_ACTIONS_WHILE_EDITING = [ | |
'view-post', | ||
'view-post-revisions', | ||
'rename-post', | ||
'rename-template', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll ramble a bit and might be missing things, and I will check the code and changes better, but it seems too complex to me at first glance. I don't get the end goal with some of this code. Is it to have for example a single I think it's apparent that we need to reuse some actions and know some context (such as 'edit' mode, or 'post vs site' editor for redirections). Can we extract this info somehow to avoid such explicit declarations? If we cannot avoid that, shouldn't it be part of the As I said I'll have to look more the code to get a better understanding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think your outside perspective is very useful, don't ignore your intuition. We need more red teams, in fact. Taking a step back, there are several things we can do, as well as some questions to ask ourselves now that we have concrete experience with the unification:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems I had accidentally deleted a small part of my first comment. My main issue is what we are trying to unify.
That's one of the issues here. This PR (and at least this merged one) is not about DataViews, but it tries to do what DataViews do. I worked on the DataViews actions API and you can say it consists of two main parts:
The component that renders the actions is reused from all consumers, but the actions are provided by the consumers. What I'm trying to say is that we should probably have a reusable component to render the actions, but the actions unification is a whole different matter and the unification of them would involve questions like:
Right now in trunk and with this PR, we are taking an approach where we duplicate (almost) the rendering component, which is not necessarily bad to start with, if we don't want the UI to match, but if we want it to match we could consider extracting this to a private component package. Again it's probably fine to just duplicate for now and update the comments to have in mind if we'll need to create a shared component. I think we should clarify the vision about actions unification. I don't think it's great to have Finally maybe this is the time that we should also just think about a bit, how the actions API is similar to the commands API and if it would be possible to converge/unify a bit there or even take inspiration, or even see gaps in that API 😄 To sum up, I think we should take things step by step and try to avoid unnecessary complexity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This reminded me of Riad's proposal: a single API to describe actions/commands and reuse them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @ntsekouras,
The end goal of this code is to provide the same actions on dataviews, edit site inspector, edit site site details panel, and edit post inspector. Which was on mockups. This means that actions like delete template and revert a template should be available on edit post too. So the editor package seemed to be the best package to have the actions.
Yes in the future we may join the delete-post, delete-template, and delete-pattern action in a single action that can be applied to every post. It leaves the possibility for that and I think it may make sense. But it is not the goal. My goal was just not to duplicate the code of the actions while keeping exactly the actions API (object structure the same as before).
While editing for UX purposes we don't want some actions shown. usePostActions allows the consumer to select the actions that wants to use in a given case. Previously the consumer could also import some actions and not others. usePostActions is like an import of actions. I agree in the future we may have a single renameItem or deleteItem that can be done as a follow-up.
The command API and actions API are very similar and we already have some inconsistencies, like on the actions we ask for user confirmation on some destructive actions, and on commands we don't. We can explore making the actions available as a command or on the command code callbacks call the action callbacks and or RenderModal. The points you raised are valid and are things we can look. The set of action-related PR's I worked on focused on a specific goal to bring the same actions to the 4 different places we have. I agree sometimes duplication is better than adding complexity and for example, I opted to duplicate the UI that renders the actions menus, but that is only 150 lines of code. And I feel that code does not make sense as a WordPress component and exposing it as a private package just increases the maitainability. It seems to reuse the actions across the different places we could have duplicated them or moved them to a package where they can be reused. I opted for the latter and that was done without any changes added to the actions API. The actions object passed to the dataviews is exactly the same (this PR or the one it depends on #60395 did not touch the dataviews package at all). The only artifact that was added is a usePostActions in the editor package that returns an array of action objects with the same shape we already have. That artifact is private and can easily be changed or even removed, it just exists as a way to export actions from the editor package to edit-site and edit-post. The artifact just provides an easier way to import an array of actions. Instead of import{ usePostActions } usePostActions( onActionPerformed, [delete-template, revert-template]), we could have import{ deleteTemplate, revertTemplate}; useMemo( add my action performed callback to the delete action; [deleteTemplate,revertTemplate] ); and not have usePostActions at all we would just repeat some code inside useMemo. I think duplicating all the actions code would not have been a good choice, as the code has exactly the same goal and we have a place where we can put that code that makes sense and allows that reusability. The fact that actions could just work across the for different places with only minimal checks added in isEligible keeping the same action object passed to dataviews without changes there also is an argument in favor of reusing them. I understand it may feel strange to have 'rename-post', 'rename-template', on the same array. But both actions exist and that array can have any action that is available. Previously I could also have somewhere import {renamePost, renameTemplate}. Probably we should not have had different actions to rename a post, a template, and a pattern and it would have been better to just have a single rename action. That was an issue that we already had. I can have a look into joining some of the actions I think that would be a good code-quality enhancement. Do you think exploring joint rename/delete|post/template in a follow-up or a parallel PR would be enough to unblock the current PR? Or are there any other changes you think we should do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @mcsf,
I feel we can not take out onActionPerformed it is very dependent on the context of where the action is executed. E.g: when I delete a template that is assigned to a page I should not navigate anywhere I should just switch to page editing and assign the default template to that page, on data views when a template is deleted I should do nothing at all, and on site editor, I should navigate to the template list. In the view of another plugin that also offers a way to delete templates, I may need to do something totally different. Detecting all these cases and doing the right thing all the time seems "too magic" to me. But I'm open to suggestions of other things we can remove, and or enhancements we could do. The onActionPerformed is part of usePostActions which is private and leaves the door open for enhancements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Probably yes, but are we 100% sure that every DataViews action should be the same? Probably they should.. It's good to have them in editor package and that wasn't the problem for me.
I think we should do that and that's the goal. I agree we can do it iteratively, but that means we should separate these actions for start (ex. Template actions, post actions, etc..) and then (if not from that iteration) have shared actions. For me the complexity stems from the fact that we're trying to infer some info in places but on the same time we have hardcoded strings/actions etc.. I think for start we should:
|
||
'move-to-trash', | ||
'reset-template', | ||
'delete-template', | ||
]; | ||
|
||
export default function PostActions( { onActionPerformed, buttonProps } ) { | ||
|
@@ -51,13 +50,7 @@ export default function PostActions( { onActionPerformed, buttonProps } ) { | |
POST_ACTIONS_WHILE_EDITING | ||
); | ||
|
||
if ( | ||
[ | ||
TEMPLATE_POST_TYPE, | ||
TEMPLATE_PART_POST_TYPE, | ||
PATTERN_POST_TYPE, | ||
].includes( postType ) | ||
) { | ||
if ( PATTERN_POST_TYPE === postType ) { | ||
return null; | ||
} | ||
return ( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem in line with the latest changes (#60667), nor with the deleted hunk of this patch. I expected something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm right, the same comment applies to other similar parts of the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as of #60689 and related work, the parts should go back always to
/patterns
. We should not use/wp_template_part/all
anymore, it only exists (temporarily) for hybrid themes (classic themes that declareblock-template-parts
theme support), so they can access that particular site editor screen.I've also checked that hybrid themes cannot access the delete action anywhere (index page, details page, editor), in case we needed to go back to a different place if the theme was hybrid. Unless I'm missing some flows, we don't need that action for those themes.