Allow template duplication + concept of active templates #67125
Conversation
|
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. |
9e9eafc to
1d75cf3
Compare
|
Size Change: +1.31 kB (+0.07%) Total Size: 1.89 MB
ℹ️ View Unchanged
|
| name === 'wp_template' && | ||
| typeof key === 'string' | ||
| ) { | ||
| name = '_wp_static_template'; |
There was a problem hiding this comment.
Is the _ at the beginning necessary?
There was a problem hiding this comment.
We can change it to whatever we want. I was wondering though if we should make the endpoint private by making it a random path. But the post type can stay the same.
| @@ -187,9 +296,13 @@ export default function PageTemplates() { | |||
| context: 'list', | |||
| } ); | |||
| const editAction = useEditPostAction(); | |||
| const setActiveTemplateAction = useSetActiveTemplateAction(); | |||
There was a problem hiding this comment.
Why is this action defined here and not in the "fields" package like all the other actions?
There was a problem hiding this comment.
Cause I didn't really know where they should be defined.
205296d to
0676add
Compare
|
Flaky tests detected in 0cb1a2d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12558533055
|
|
Pinging @WordPress/block-themers and @WordPress/outreach to get as many eyes on this as possible :) And help testing this would be much appreciated! |
|
Using 2025, I removed all my custom templates and I see this in the theme templates: My understanding is that this one is marked "inactive" because it's not a "hierarchy" template, My feeling is that it doesn't mean entirely "inactive", it may be already assigned to some pages. So this raises two questions:
That said, It's not easy because "hierarchy" doesn't really mean anything to most users. |
|
One possible option would be to consider these "custom page templates" as always active (you can't really disable them). I noticed that right now, you can active/deactive them but it doesn't do anything really and they don't show up in "active templates" either. |
|
@youknowriad Yes, we should think about those.
Maybe some in-between state. "Active on a page" would be a bit long. We're seem to be calling these elsewhere "for a specific item", so maybe a gold badge with "Specific"? Not sure. It would be good if you can hover over the badge and see more info.
Yes, it's complex, because there's two scenarios: either you assign an existing template to be the page template. In which case the template in not active except for this specific page. Or you create a new specific template that has the slug I think we should do something here in this PR, but there's probably some details left to figure out later. |
|
For example, one option is to deprecate |
|
I found myself missing a way to name the templates that I create. I believe we need to:
I think this is the kind of details that can be addressed in a follow-up though, once the whole mechanic and UX is agreed upon. |
|
This is very cool. Some initial observations: After building this branch, a bunch of templates appeared in the 'Custom templates' section, many of which are duplicates of the same template. It's not clear why. Perhaps they're revisions? The point being that we should think about the upgrade process, expected results etc. Something feels wrong with the logic around the active state of the Front Page template. I have one (several actually, see previous point) in 'Custom templates', but none of them are active. It doesn't appear in 'Active templates' either. It seems to be ignoring the template hierarchy, maybe that's intended? :) I'm able to click into and 'edit' theme templates. Two issues around this; it's not clear what happens, I assume the template is duplicated, but is it also activated? It's inconsistent with theme patterns, which are entirely uneditable until duplicated. In general I think this part of the UX needs more thought, but for the initial implementation perhaps they should be consistent to help unify the concept? Potentially a big can of worms, but communicating the template hierarchy in the UI is quite challenging :) As a provocation, and to define the scope of design, do we need to do that with custom templates? Theoretically could any template be assigned to any page? I suppose this relates to the previous point about the 'Front Page' template. For The A couple of very small points on the design;
|
That is weird, it shouldn't be happening since you can only have one custom template per "hierarchy slug" in trunk. I wonder if you had some lingering broken templates (that were hidden in trunk) from very old branches or something.
I think it probably relates to the previous point. I think upon upgrade, all user templates should be "active" by default.
I agree, I think for me the ideal is to be explicit about what's happening (no edit) but there are some challenges with that:
|
| @@ -170,7 +171,7 @@ const getNavigationCommandLoaderPerTemplate = ( templateType ) => | |||
| return { | |||
| isBlockBasedTheme: | |||
| select( coreStore ).getCurrentTheme()?.is_block_theme, | |||
| canCreateTemplate: select( coreStore ).canUser( 'create', { | |||
| canCreateTemplate: select( coreStore ).canUser( 'read', { | |||
There was a problem hiding this comment.
Why did we change the perm? Seems natural to check for create in canCreate...
There was a problem hiding this comment.
Why? The commands are meant to navigate to templates, not create new templates?
There was a problem hiding this comment.
But yes this variable should be renamed 😃
There was a problem hiding this comment.
I agree that using the create capability check here is odd but necessary. Users who can read templates might not have access to the Site Editor.
See #60317 (comment).
P.S. We should extract similar minor and unrelated fixes into separate PRs. There's already a lot going on here and that would make reviewing bit easier.
There was a problem hiding this comment.
Then this check should be called differently and we should use something else to check if the user has access to the site editor. This is not a good proxy for checking site editor access, it is more restrictive now. Also we should then use it as a condition to prevent all site editor commands from loading?
There was a problem hiding this comment.
Also we should then use it as a condition to prevent all site editor commands from loading?
It's hard to generalize it. The Site Editor needs more granular capability checks, but those should be mapped out separately - maybe even incorporated into the router.
As I mentioned, I would remove and extract similar fixes into separate PRs.
There was a problem hiding this comment.
Yes, here the change is necessary because you cannot create static templates through the endpoint, you can only create custom templates.
There was a problem hiding this comment.
So I guess we need another additional check, not sure what
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @erikjoling. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
I'd love to get this merged, but let's see more opinions on it. I think most bugs and issues will come from things that weren't previously possible before, so honestly I don't think it's a big deal. |
I feel like the simplest for users would be to just "always disable for everything" (hierarchy and specific pages) regardless of template. |
|
Apologies as I haven't caught up on the above feedback but wanted to take some time to explore this. In testing this, it's not clear that when deactivating a template where that template then goes. I eventually realized, I needed to look at "Twenty Twenty Five" but I don't think that's clear. I also would expect more friction in deactivating a template with some sort of a warning considering this isn't an action I expect people to take very often and it is, in a way, destructive. unclear.where.deactivated.template.goes.movRelated to this, I find duplicating to be a bit confusing as is since you duplicate but then don't see the template you just created. Perhaps we can have the snackbar have a link where you can see where the template you just took action on went? duplicate.template.movI find the differences in three dot menus to be confusing as well, depending on if you're looking at active templates vs custom templates. Shouldn't we at least show an edit option in both places? three.dot.menu.movIn any case, when we do merge this, I'd encourage putting it behind an experimental flag potentially and doing a call for testing (I can help with that) to refine it a bit further before releasing it fully. I imagine there's stuff we haven't yet caught. |
|
Is there a way to split this PR into smaller changes that are easier to refresh and review? |
0cb1a2d to
a265324
Compare
| $paths[] = '/wp/v2/templates/lookup?slug=home'; | ||
| // There's an issue where preloaded data does not invalidate. | ||
| // $paths[] = '/wp/v2/templates/lookup?slug=front-page'; | ||
| // $paths[] = '/wp/v2/templates/lookup?slug=home'; |
There was a problem hiding this comment.
To do: fix this. Need to check again what the reason was.
| @@ -36,6 +36,9 @@ function gutenberg_is_experiment_enabled( $name ) { | |||
| } | |||
|
|
|||
| // WordPress 6.8 compat. | |||
| require __DIR__ . '/compat/wordpress-6.8/class-gutenberg-rest-static-templates-controller.php'; | |||
| require __DIR__ . '/compat/wordpress-6.8/class-gutenberg-rest-templates-controller.php'; | |||
| require __DIR__ . '/compat/wordpress-6.8/template-activate.php'; | |||
There was a problem hiding this comment.
To do: move to 6.9 compat.
| @@ -45,9 +57,15 @@ const defaultLayouts = { | |||
| }, | |||
| [ LAYOUT_GRID ]: { | |||
| showMedia: true, | |||
| layout: { | |||
| badgeFields: [ 'active', 'slug' ], | |||
There was a problem hiding this comment.
Instead of introducing __returnBadge we could configure the badge component via this config. For example:
{
layout: {
badgeFields: [ { field: 'active', intent: 'something' }, 'slug' ]
}
}There was a problem hiding this comment.
Yes, but this intent depends on the item 🙂
There was a problem hiding this comment.
I like the direction and distinction this PR brings! It becomes more intuitive to handle and manage templates! ❤️
I'm doing some testing against WooCommerce and here are some findings.
Incorrect active template
Scenario:
- I have a WooCommerce template I've edited.
- Once I enable changes from this branch I can see:
- default template in WooCommerce section
- my edits in "Custom Templates" section
- However, default template is "Active" and default is displayed on the frontend
Expected: I'd expect the latest revision from "Custom Templates" to be "Active" and not the default one.
Do you see where it may be coming from?
"Active templates" don't list plugin templates
I only see theme templates in "Active Templates" tab and no plugin or custom templates. Is that expected?
getEditedPostId (deprecated) may return number instead of string
getEditedPostId is deprecated but WooCommerce still relies on it, that's how I spotted the breaking change. With this PR it may return number instead of string (cannot reproduce on trunk)
- I'll make adjustments on WooCommerce side to stop relying on deprecated methods (
getEditedPostIdandgetEditedPostType). - Since it's deprecated, I assume a breaking change is fine. But still wanted to mention it.
This are some issues I spotted already. The issue with getEditedPostId is strongly disrupting testing with too many console errors and influencing the performance. I'm focusing on it and will come back with another round of testing from WooCommerce/plugin POV 🙌
Creating a new PR to squash all commits and preserve the history of #66951.
What?
Fixes #66950.
Why?
See above.
How?
active_templatesthat contains a maps of slugs to active template IDs. To disable default templates, it can be set tofalse.auto-draftuser template is created behind the scenes in case edits are made. If no edits are made this disappears. The active template is also set to the draft. If the user makes edits and wants to save it, changing active templates can be unticked in the multi-entity saving panel. There's potential to improve the UX here with this new paradigm.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2024-11-13.at.09.20.16.mov