-
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
Add fallback content when creating templates according to template hierarchy #37054
Conversation
Size Change: +153 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
archive: 'category', | ||
category: 'index', |
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.
I think this is slightly incorrect. A new category template should use archive as the fallback. A new archive template should use index as the fallback.
Edit: We don't currently allow the creation of category templates at all, so perhaps that can be omitted for now?
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.
and if neither archive or category exist, archive should fall back to index too
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.
BTW I just found a helper that already does something like this:
Line 21 in 2e5af3f
export function isTemplateSuperseded( slug, existingSlugs, showOnFront ) { |
It's using this const
:
gutenberg/packages/edit-site/src/components/navigation-sidebar/navigation-panel/constants.js
Lines 64 to 68 in 5562482
export const TEMPLATE_OVERRIDES = { | |
singular: [ 'single', 'page' ], | |
index: [ 'archive', '404', 'search', 'singular', 'home' ], | |
home: [ 'front-page' ], | |
}; |
We might wanna reuse that (unless we decide to go with #37258 😊).
I can't comment on whether this is the best approach, but it seems to be working at least :) |
Downloading the Gutenberg PR build. In general (testing this PR nor not) what I see is adding any new template the content will look like the index template content. It requires one to delete the content and add the content one needs. Search template would likely add a Heading, a Search block placeholder and a Paragraph block. Archive template would likely add a Heading, Archive block (I think we have one) and some additional text. |
What do you think @ockham? |
👋 (Sorry, I only noticed the ping now.) This is an interesting issue 🤔 My gut reaction is that I'd rather avoid encoding the template hierarchy on the client side. The template hierarchy is a tricky beast, and I feel it's better to have only one source of truth for it (on the server side, as opposed to duplicating it client-side).
Those sound like better options to me. We already hook into various points to run the template hierarchy resolution algorithm (e.g.), so we might find a point that lends itself to doing this for template creation. I'll have a look tomorrow unless someone else beats me to it 🙂 |
@ockham Awesome! If that's possible then I think we should close this PR and favor that one :) |
@kevin940726 I looked into this today, but haven't totally gotten to the bottom of it yet. I was unaware that the "Add New" button on the templates page... ...actually creates templates explicitly, i.e. by (FWIW, this is different from what happens when the user attempts to edit a template that's provided by a theme's block template file -- such as the Archive template in the screenshot above: In this case, the editor loads the template from that file without creating a I'm not totally sure about the explicit creation. There's a point to be made that templates (i.e. OTOH, it's fair to acknowledge that the implicit I'll mull this over a bit more and would like to hear @youknowriad's opinion on this. I think it's worth to at least experiment with an approach that disables explicit template creation, and makes sure that the REST API controller's |
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.
I don't have a strong opinion on whether the fallback should be server side or front-end personally.
@@ -52,13 +86,18 @@ export default function NewTemplate( { postType } ) { | |||
slug, | |||
} ); | |||
|
|||
const newTemplateContent = | |||
defaultBlockTemplate ?? | |||
getFallbackTemplateContentFromHierarchy( slug, templates ); |
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.
It's a bit weird to rely on existing templates to create a fallback template content? Is this how it's supposed to work or should we actually provide a list of default fallbacks?
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.
The motivation for using existing templates is to ensure that the new template matches the current theme design (IE include the appropriate header / footer template parts and general block structure).
A much simpler approach would be to prescribe a template including the base header / footer template parts, then the user would need to select which one to use whilst creating the template. So a new search template might look like:
The drawback here is that if the theme has a complex layout (maybe the header is actually a sidebar) then this won't match at all.
@@ -41,6 +73,8 @@ export default function NewTemplate( { postType } ) { | |||
defaultTemplateTypes: select( | |||
editorStore | |||
).__experimentalGetDefaultTemplateTypes(), | |||
defaultBlockTemplate: select( editorStore ).getEditorSettings() |
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 was initially an API suited for "custom templates" and not "hierarchy templates". Is it fine to use it consistently no matter the template?
Also should we sync the logic here with the fallback done in the template mode? (post editor)
Some early-stage experiment here: #37258 |
Maybe best to punt this to 5.9.1? I'm not feeling a whole lot of consensus on the approach. Just FYI, today (Monday) is the last day for this PR to be merged if it is to ship in WP 5.9. I'll begin backporting merged PRs on Tuesday morning Australian Eastern time. |
It would be a real shame if this wasn't included, especially as we had something which did roughly the same job in the past. The original issue really undermines the add-template flows. |
We're out of time unfortunately. I've swapped the labels so that we include this in 5.9.1. |
Hey @kevin940726, thanks for the ping! Well I'm a little biased since #37258 is my PR, so yeah, I would like to see it merged 😊 However, there was some disagreement on the implementation there, and reviews at some point stalled 😅 |
This can probably be closed now that #42520 has been merged. |
Description
Close #36648.
Add fallback content when creating templates according to "Template Hierarchy". TBH, I'm not sure if this is the right approach. Feels like it might be better if there's a API to get the fallback content (or just bake it right into the create request?). So that we don't have to maintain a (pseudo-)mapping on front-end, but rather leverage the actual algorithm on back-end to populate the default content.
How has this been tested?
tt1-blocks
themeTypes of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).