Post Type Actions: Unify the list of available actions#61520
Merged
Conversation
youknowriad
commented
May 9, 2024
youknowriad
left a comment
Contributor
Author
There was a problem hiding this comment.
I realize this moves away from the "cherry-picking" aspect that we've landed on before, but I think it's for the better.
|
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. |
youknowriad
commented
May 9, 2024
| [ history ] | ||
| ); | ||
| const actions = usePostActions( onActionPerformed, PAGE_ACTIONS ); | ||
| const actions = usePostActions( onActionPerformed ); |
Contributor
Author
There was a problem hiding this comment.
This hook should be receiving the postType, otherwise it will give inconsistent results.
Contributor
Author
There was a problem hiding this comment.
Also, I think this hook is misplaced it should be in "core-data" and not in "editor".
youknowriad
commented
May 9, 2024
| ! isTemplateOrTemplatePart && renamePostAction, | ||
| isTemplateOrTemplatePart && renameTemplateAction, | ||
| ! isTemplateOrTemplatePart && trashPostAction, | ||
| ].filter( Boolean ); |
Contributor
Author
There was a problem hiding this comment.
This is where the filtering of the actions based on post type happens. Ideally we should be checking post type slugs but supports (like we do for postType.viewable).
Also ideally some actions should merge like (rename template and rename post...) but I'm leaving that for later.
Member
There was a problem hiding this comment.
The checking for postType.viewable, and even things like rules e.g.: can delete can create another post, etc should be done on isEligible I think.
Contributor
Author
There was a problem hiding this comment.
Not entirely sure about postType.viewable #61520 (comment)
|
Size Change: +1.77 kB (+0.1%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
youknowriad
commented
May 9, 2024
| renameAction, | ||
| resetAction, | ||
| deleteAction, | ||
| duplicatePatternAction, |
Contributor
Author
There was a problem hiding this comment.
All of these actions need to be moved to the editor/post-actions hook rather than being specific to dataviews. I'm not doing that in this PR though.
f034822 to
4b6b24f
Compare
| ) | ||
| : defaultActions; | ||
| const actions = [ | ||
| isTemplateOrTemplatePart && resetTemplateAction, |
Member
There was a problem hiding this comment.
I wonder if it would not be better to do the checks for post type on the isEligible function of the action it self.
Contributor
Author
There was a problem hiding this comment.
I thought about it, I do think it would be better but I'm not sure that the current API allows us to do that. The isEligible can't call selectors with resolvers at the moment and be certain that the value is there.
Contributor
Author
There was a problem hiding this comment.
I'm also not sure 100% that it's better. This is the mental model I have for now:
- isEligible is for actions that can apply to one item of the current dataviews/list but may not apply for a reason that is specific to the current item.
- The actions you pass to DataViews (or the Actions component) are all "valid" for the current post type.
Member
There was a problem hiding this comment.
On the item itself, the rest API provides some checks in wp:action... that allows some possibilities but yes it is very limited.
I imagine a plugin may want to build a view that shows all posts from all types in the database in that case we would want all actions there, and depending on the item some would be available, and some would not.
But given that we are keeping actions as hooks we can keep the current approach the current mental modal long term maybe isEligible should be an async function.
| }; | ||
|
|
||
| export function usePostActions( onActionPerformed, actionIds = null ) { | ||
| export function usePostActions( postType, onActionPerformed ) { |
Member
There was a problem hiding this comment.
I liked the previous flexibility of allowing the consumer to select the actions. I think we will end up needing to retrieve only some actions in some cases. But I'm fine with using postType for now until we see if that need materializes.
Contributor
Author
There was a problem hiding this comment.
I think flexibility comes with inconsistency. I'd like to avoid it until we find our selves in a situation where we want to pick what we want. Users can still pick what they want, it's just a simple filter on top of the returned value.
jorgefilipecosta
approved these changes
May 10, 2024
jorgefilipecosta
left a comment
Member
There was a problem hiding this comment.
Things tested well for me I did not notice any regression, and the code looks good 👍
This was referenced May 10, 2024
mcsf
reviewed
May 10, 2024
| [ 'edit-post', 'view-post-revisions' ] | ||
| ); | ||
| const templatePartActions = usePostActions( TEMPLATE_PART_POST_TYPE ); | ||
| const patternActions = usePostActions( PATTERN_TYPES.user ); |
Contributor
There was a problem hiding this comment.
I'm ever more convinced we should drop these constants across patterns, dataviews, etc. It's easy to waste 30 seconds, which become 1.5 minutes with the ensuing distractions, especially when the LSP can't trace the identifier to its type/definition due to the opaque unlock membrane... just to confirm that PATTERN_TYPES.user is 'wp_block'. These aren't obscure values, they are standard post types and other such values.
Contributor
Author
There was a problem hiding this comment.
Maybe open a PR and see what folks say about it :)
ntsekouras
reviewed
May 10, 2024
ntsekouras
left a comment
Contributor
There was a problem hiding this comment.
That was quick! Nice work.
I echo that 😄
Thanks Riad! I think we should keep track of the remaining follow ups that are needed, like moving some actions to different packages, reusing some actions like the renamePostAction, etc.. I like that some things are clearer now like we don't have to pass around specific action ids and filter all the available actions.
I'm wondering if we'll come across use cases where we want different actions in Data Views and Post Card panel actions for the same post type and how we'll accommodate this with this API, but I guess future will tell 😄
| return { | ||
| postType: getCurrentPostType(), | ||
| item: getCurrentPost(), | ||
| postType: getCurrentPostType(), |
Contributor
There was a problem hiding this comment.
I know this existed here, but I've commented about it elsewhere too. I think it's probably better to use the edited entity, because many actions might be better to use the current info, even if not saved. Examples would be the rename Post action, or the duplicate post action.
Contributor
Author
There was a problem hiding this comment.
Definitely :) Good catch.
Contributor
Author
|
So here are the follow-ups that I can think of:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
While working on the unification of the editor sidebar between the post and site editors, I noticed that we have two separate components to render the "Actions" of a given post type: PostActions and TemplateActions in the site editor. I think we should only have a single component that is responsible of rendering the actions for a given post type. We shouldn't have to be making decisions randomly, instead the post type should give us enough information to know what action is available or not.
This will also help with extensibility since we'll be able to filter the available actions in a single place per post type.
Note This is affects both the actions panel in the editor and the actions in the dataviews for these post types. Some actions don't make sense in both context (like edit) and will need to be moved out of the generic place. So this is not ready entirely.
Testing Instructions
1- Test the Actions menu for all post types (pages, posts, template, template part, pattern)
2- Test in both dataviews and editor.