Update: Reuse and unify template actions. #60754
Conversation
|
Size Change: -156 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
| items[ 0 ].type === TEMPLATE_PART_POST_TYPE | ||
| ? '/' + TEMPLATE_PART_POST_TYPE + '/all' | ||
| : '/' + items[ 0 ].type, |
There was a problem hiding this comment.
This doesn't seem in line with the latest changes (#60667), nor with the deleted hunk of this patch. I expected something like:
items[ 0 ].type === TEMPLATE_PART_POST_TYPE
? '/patterns'
: '/' + items[ 0 ].typeThere was a problem hiding this comment.
If I'm right, the same comment applies to other similar parts of the patch.
There was a problem hiding this comment.
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 declare block-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.
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| if ( ! template ) { | ||
| if ( | ||
| ! template || | ||
| ( template.type !== TEMPLATE_POST_TYPE && |
There was a problem hiding this comment.
This feels weird to be inside a function called isTemplateRemovable.
There was a problem hiding this comment.
Good catch 😅 I will fix this.
| @@ -36,7 +36,10 @@ const POST_ACTIONS_WHILE_EDITING = [ | |||
| 'view-post', | |||
| 'view-post-revisions', | |||
| 'rename-post', | |||
| 'rename-template', | |||
There was a problem hiding this comment.
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 delete-post action and apply to every post type and check if isEligible? How the edit scope affects the implementation and what other scopes we can have?
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 actions API?
As I said I'll have to look more the code to get a better understanding.
There was a problem hiding this comment.
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 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
actionsAPI?
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:
- You touched on the idea of context (
edit,post, etc.). You also touched on the idea of a generic action provider potentially conflicting with type-specific requirements: is it a code smell if a function calledisTemplateRevertableneeds to validate its argument as template-like? - Meanwhile, the current system is very flexible in that each action is a set of methods callable on any data:
isEligible( item ),onActionPerformed: (actionId, items) => switch (actionId) {.... So we are defaulting to giving consumers almost maximum control with the goal of making Data Views versatile and post-type-agnostic. - Now that we've put things in, how much can we take out? Could we do the exercise of removing as much of that control as possible, with the goal of minimising moving parts? Then we would be forced to make assumptions about the data and the permissible set of actions. For example, how do we reuse actions across screens and entities (and how much should we)? As another example, what would it take to kill
onActionPerformedand let the system navigate when necessary or intelligently perform other kinds of post-action cleanup?
There was a problem hiding this comment.
It seems I had accidentally deleted a small part of my first comment. My main issue is what we are trying to unify.
Meanwhile, the current system is very flexible in that each action is a set of methods callable on any data: isEligible( item ), onActionPerformed: (actionId, items) => switch (actionId) {.... So we are defaulting to giving consumers almost maximum control with the goal of making Data Views versatile and post-type-agnostic.
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 actions API - how we define actions, isEligible, RenderModal, etc..
- The component that takes as input the above actions and can render them in form of a dialog, inline, etc.. based on the action's properties.
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:
is it to have for example a single delete-post action and apply to every post type and check if isEligible
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 renamePostAction, renameTemplateAction in the same array. It feels weird to me that we already have override actionIds in usePostActions and POST_ACTIONS_WHILE_EDITING to pass. It feels like we don't have clear answers about the use cases and we rush into abstractions, which add complexity. We should either for start have a single rename action etc.. or we could just start by separating the actions per our conditions (probably by checking post type? 🤔 ). If the context matters, we should also check this if/how to incorporate this..
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.
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
This reminded me of Riad's proposal: a single API to describe actions/commands and reuse them.
There was a problem hiding this comment.
Hi @ntsekouras,
I don't get the end goal with some of this code.
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.
Is it to have for example a single delete-post action and apply to every post type and check if isEligible? How the edit scope affect the implementation and what other scopes we can have?
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).
I think we should clarify the vision about actions unification. I don't think it's great to have renamePostAction, renameTemplateAction in the same array. It feels weird to me that we already have override actionIds in usePostActions and POST_ACTIONS_WHILE_EDITING to pass. It feels like we don't have clear answers about the use cases and we rush into abstractions, which add complexity. We should either for start have a single rename action etc.. or we could just start by separating the actions per our conditions (probably by checking post type? 🤔 ). If the context matters, we should also check this if/how to incorporate this..
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.
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 😄
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.
If the actions are on the editor package it will also make it easy to add commands like revert template to edit post which for now just exist on the edit site package. These commands could just call the actions.
But that does not seem related to this PR as I'm not changing the actions API.
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.
For the actions them selfs the goal is exactly the same across the 4 use cases (dataviews, edit site inspector, edit site site details panel) given an item or set of items check if some action is executable on it, execute the action or render UI to confirm its execution.
Given that the goal is exactly the same I don't think it makes sense to repeat 1200 lines of code (+- 800 for now but will increase when the 400 lines of pattern actions are also moved). What was happening before was that for each place where we needed an action, it was being reimplemented from scratch e.g.: #60232.
@Mamaduka brings up a good point, we will need to check for capabilities e.g: does the current user have delete capabilities? I think if actions are in a single place it is easier to add these capability checks
check than if they are repeated across multiple places (we will end up adding checks in some places but missing them in other places like currently we ask user confirmation on some places but don't on others).
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.
Given that the actions API is the same, where do you feel that complexity was added?
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.
Hi @mcsf,
Adding the template check on isTemplateRevertable does not make sense at all 😅 It is something I will change.
Now that we've put things in, how much can we take out? Could we do the exercise of removing as much of that control as possible, with the goal of minimising moving parts? Then we would be forced to make assumptions about the data and the permissible set of actions. For example, how do we reuse actions across screens and entities (and how much should we)? As another example, what would it take to kill onActionPerformed and let the system navigate when necessary or intelligently perform other kinds of post-action cleanup?
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.
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.
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.
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.
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 the actions to editor package and organise(export) them based on post type probably
- Let the consumer pass the actions and have a dumb component to render actions
|
I noticed that some of the Let's take the "Move to Trash" action as an example. It only checks post status and ignores user capabilities. If I remove the Steps to reproduce:
You can restore the role to default capabilities by running - |
jorgefilipecosta
force-pushed
the
fix/reuse-and-unify-template-actions-
branch
from
bfab2f0 to
8a6a528
Compare
Motivation ========== Actions and commands -------------------- In the context of Data Views, there has been a lot of recent work towards providing a set of actions operating on posts, templates, patterns (e.g. rename post, edit post, duplicate template), and ultimately other entities. These actions, however, aren't unique to Data Views, and indeed exist in several different contexts (e.g. Site Editor inner sidebar, new Admin "shell" sidebar, Pages index view, Post Editor), so the next step was to unify actions across packages (e.g. #60486, #60754). The first unification effort led to an abstraction around a hook, `usePostActions`, but the consensus now is to remove it and expose the actions directly (#61040). Meanwhile, it has been noted that there is a strong parallel between these _actions_ and the Command Palette's _commands_, which has its own API already. This isn't a 1:1 mapping, but we should determine what the overlap is. Actions and side effects ------------------------ There is a limit to how much we can unify, because the context in which actions are triggered will determine what secondary effects are desired. For example, trashing a post inside the post editor should result in the client navigating elsewhere (e.g. edit.php), but there should be no such effect when trashing from a Data View index. The current solution for this is to let consumers of the `PostActions` component pass a callback as `onActionPerformed`. It works but there's a risk that it's too flexible, so I kept wondering about what kind of generalisations we could make here before we opened this up as an API. Extensibility ------------- As tracked in #61084, our system -- what ever it turns to be -- needs to become extensible soon. Somewhere in our GitHub conversations there was a suggestion to set up an imperative API like `registerAction` that third parties could leverage. I think that's fair, though we'll need to determine what kind of registry we want (scope and plurality). An imperative API that can be called in an initialisation step rather than as a call inside the render tree (e.g. `<Provider value=...>` or `useRegisterAction(...)`) is more convenient for developers, but introduces indirection. In this scenario, how do we implement those aforementioned _contextual side effects_ (e.g. navigate to page)? The experiment ============== It was in this context that I had the terrible thought of leveraging wp.hooks to provide a private API (to dogfood in Gutenberg core packages). But, of course, hooks are keyed by strings, and so they are necessarily public -- i.e., a third party can call `applyFilters('privateFilter'`, even if `privateFilter` is not meant to be used outside of core. This branch changes that assumption: hook names *must* be strings, *except* if they match a small set of hard-coded symbols. These symbols are only accessible via the lock/unlock API powered by the `private-apis` package. Thus, core packages can communicate amongst each other via hooks that no third party can use. For example: - An action triggers `doAction` with a symbol corresponding to its name (e.g. `postActions.renamePost`). - A consumer of actions, like the Page index view (PagePages), triggers a more contextual action (e.g. `pagePages.renamePost`). - A different component hooks to one of these actions, according to the intended specificity, to trigger a side effect like navigation. See for yourself: upon `pagePages.editPost`, the necessary navigation to said post is triggered by a subscriber of that action. Assessment ========== Having tried it, I think this is a poor idea. "Private hooks" as a concept is a cool way to see how far `private-apis` can take us, but they seem like the wrong tool for the current problem. Still, I wanted to share the work, hence this verbose commit. I think our next steps should be: - Finish the actions refactor (#61040) - Impose constraints on ourselves to try to achieve our feature goals with less powerful constructs than `onActionPerformed`. I'm still convinced we haven't done enough work to generalise side effects. Consider it along with the commands API. - Try a more classic registry-based approach for actions (`registerAction`)
|
Closing as the unification was done at #61520. |
Similar to #60486.
Reuses the template actions across the post editor inspector, side editor inspector, details panel, and the dataviews.
In progress
The handle of deleting a template that is associated with a page is not yet implemented.
Screenshots
Testing Instructions
Create a custom template on the templates section.
Renamed the template using the edit site inspector sidebar.
Renamed the template using the details panel.
Tried to delete a template using both the edit site inspector sidebar and the details panel.
Added a change to a theme template and tried the reset action on both the edit site inspector sidebar and the details panel.
Associated a theme template with a change to a page, and verified I could reset the template on the edit post.