-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Edit Site: Add multiple template loading #19141
Conversation
@@ -69,6 +90,17 @@ export default function BlockEditor( { settings: _settings } ) { | |||
onChange={ setContent } | |||
> | |||
<BlockEditorKeyboardShortcuts /> | |||
<Sidebar.TemplatesFill> | |||
<TemplateSwitcher |
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.
Can we move the template switcher to the editor header?
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.
Actually both features, the "add template" as well.
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 wanted to do that, but we'll need to do some work on improving dropdown menus for it. I want to integrate template parts first and get it ready feature-wise for testing.
It'd be nice to develop this in conjunction with the new design stuff. There is a lot more we can do in this screen, incrementally, that we can't do in the post editor. cc @ItsJonQ @pablohoneyhoney @jasmussen
What do you think?
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 that's separate, we should still be able to use a dropdown menu from the existing components and list the current template.
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.
Sure, but it won't look like the mockup of the template switcher that was made.
I guess, it's still a step in the right direction. I'll do it when I add template parts.
Great to see this in conjunction with the entities save modal. |
This is so exciting, thanks @epiqueras! I tested this out and was wondering about two things, certainly with the understanding that this is work in progress :)
Maybe the action for adding a template could be moved to the top area, and use the current "Templates" area for only editing the active templates name / slug. This feels more in line with the mental model of the "Document" settings in the rest of the editor. |
Template parts will be managed via the template part block's edit flow. They show in the template switcher only if they are rendered in one of the loaded templates. Think of it like a zoomed-in view that you could also trigger by directly interacting with a template part block.
Yes, this is not meant to represent the final design. I just needed to expose a quick way of doing it.
I think the idea is for it to be an option in the template switcher dropdown. |
042999d
to
2e171f9
Compare
e654bb4
to
54553f2
Compare
|
2e171f9
to
f52a2f3
Compare
3222818
to
1d17971
Compare
f52a2f3
to
7f8488c
Compare
e10f411
to
e0e14bd
Compare
@@ -1,3 +1,4 @@ | |||
<!-- wp:template-part {"slug":"header","theme":"twentytwenty"} /--> |
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.
Do we need the theme slug here? It'd be good if it could be implicit to the "current theme" as set in options.
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 so that customized template parts can persist after switching themes. Different themes could both define headers.
Do you mean to make it default to the current theme if not set? We can do that, but we would need to save it on customized templates so that they don't load a different theme's template parts after a theme switch. It seems like a lot of additional implicit behaviors that might confuse people. I'm not sure saving one attribute in theme files is worth it.
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 weird requirement to have just for the sake of switching themes. It should be possible to handle it at the time of switching themes as well (also depending on what the UX looks like for what you want to preserve and what you want to replace) since we know both the previous and after contexts. Also not even sure that we want the concept of mixed parts between themes, I'd lean more towards a split of just current theme
and user 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 should be possible to handle it at the time of switching themes as well (also depending on what the UX looks like for what you want to preserve and what you want to replace) since we know both the previous and after contexts.
That is the implicit behavior I mentioned could be confusing.
Also not even sure that we want the concept of mixed parts between themes, I'd lean more towards a split of just
current theme
anduser templates
.
It would be confusing if, after switching themes, those user templates
started referencing different template parts, hence why we require the attribute after customization.
We could default to the theme that originally created each user template
, but then how does the user add new template parts to existing user 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.
I also didn't expect to see the theme slug here, since I interpreted these demo templates as a generic fallback that should work with any given theme.
It's so that customized template parts can persist after switching themes. Different themes could both define headers.
Having said that, I do agree with this part in general. It just didn't seem necessary to me in this particular case. Would it make sense to persist it only once the demo template part has been modified?
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.
What if the user modifies the template, without touching the part, switches themes, and then ends up with a different template part reference. There are too many edge cases like this where we lose with nothing to win.
0335169
to
92a57cb
Compare
I'm using the "demo templates", I've got two errors:
|
$_wp_current_template_content = null; | ||
$_wp_current_template_hierarchy = null; | ||
$_wp_current_template_part_ids = null; | ||
} |
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 would be cool to extract all the logic here into a utility function gutenberg_get_available_templates
or something like that. this bootstrap is becoming too complex.
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.
Good idea. It might be useful for plugin developers as well.
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.
Should we do this as a follow-up?
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.
Yes
885f6b7
to
8d4cbcb
Compare
I've rebased the PR and extracted the exporter. |
), | ||
[ settings.templateIds, settings.templateId, setActiveTemplateId ] | ||
) } | ||
</URLPopover.LinkViewer.Fill> |
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'm having some trouble understanding what this does?
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 lets us add options to the link viewer, here that is the button to open the template it links to, if any.
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 Still don't get it? How can we test to understand it more?
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.
Click on an internal link and see the button to edit the template it links to.
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'm having trouble getting an example wiith this. That said, I'm not really confident about the new API (fill) added to achieve this. I also feel this should have deserved a dedicated PR in order to test it, discuss it properly and come up with the best solution.
Is this a feature that was asked for in FSE designs?
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.
Ahh, you're having trouble because links are now refactored to use the new LinkControl
, so this URLPopover
is not being used anymore. Shouldn't we deprecate it?
Is this a feature that was asked for in FSE designs?
Yes
); | ||
const context = useMemo( () => ( { settings, setSettings } ), [ |
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.
Should we use a store instead to match the pattern used in all other modules core/edit-post
core/editor
...?
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 that's a bit overkill right now. We can see if we need it later, but I did it this way to avoid boilerplate.
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'm afraid if we don't do it properly now, we'll forget about it and keep adding adhoc state. I feel we should do it sonner rather than alter.
isTemplatePart={ settings.templateType === 'wp_template_part' } | ||
onActiveIdChange={ setActiveTemplateId } | ||
onActiveTemplatePartIdChange={ setActiveTemplatePartId } | ||
onAddTemplateId={ addTemplateId } |
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.
Seems to me like most of these props are useless, why not just do the switching in the component itself since you have access to the Context (or store if it's refactored)
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.
Because I didn't want to prematurely tie these components to a specific context when other UIs could use them in the future.
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 different pattern from the other components render right after it. I like the fact that the header becomes more about composing things together and keepnig the logic where it belongs (template switching in TemplateSwitcher component).
I don't really see us reusing that component differently in other places.
Nice work @epiqueras! A couple of things I noticed while testing:
|
Thanks for taking a look!
Yeah, that needs to be implemented. Probably with a REST API, I didn't want to explode this PR with.
I can save placeholders with no problem. Can you provide more info?
It's not implemented yet. It should be part of the template part block redesign. |
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.
Left a couple of comments but I think all of that can be resolved in follow-up PRs if needed. This worked nicely for me. ✨
@@ -28,7 +28,12 @@ class="edit-site" | |||
* @param string $hook Page. | |||
*/ | |||
function gutenberg_edit_site_init( $hook ) { | |||
global $_wp_current_template_id; | |||
global | |||
$_wp_current_template_id, |
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.
Curious why do we need the globals here, given that these are not coming from core WP, but are just something that's been introduced for site editor (in lib/edit-site-page.php
and lib/template-loader.php
)?
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 how we hijack template resolution to resolve block templates.
@@ -74,11 +79,53 @@ function gutenberg_edit_site_init( $hook ) { | |||
$settings['fontSizes'] = $font_sizes; | |||
} | |||
|
|||
// Get root template by trigerring `./template-loader.php`'s logic. | |||
// Get all templates by triggering `./template-loader.php`'s logic. | |||
$template_getters = array( |
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.
Are we intentionally missing get_index_template
here? Also would it make sense to refactor this in order to reuse a similar list defined here:
gutenberg/lib/template-loader.php
Lines 23 to 41 in 56c043c
$template_types = array( | |
'index', | |
'404', | |
'archive', | |
'author', | |
'category', | |
'tag', | |
'taxonomy', | |
'date', | |
// Skip 'embed' for now because it is not a regular template type. | |
'home', | |
'frontpage', | |
'privacypolicy', | |
'page', | |
'search', | |
'single', | |
'singular', | |
'attachment', | |
); |
In which case the call below becomes something like:
call_user_func( 'get_' . $template_type . '_template' );
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 needs to match the code from Core template resolution; I think it would be premature to abstract it like that right now.
$template_part_ids = array(); | ||
foreach ( $template_getters as $template_getter ) { | ||
call_user_func( $template_getter ); | ||
apply_filters( 'template_include', null ); |
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.
Should we document this filter according to WordPress documentation standards? It might make this logic easier to grok.
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.
Sure
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.
But we are not following that convention in the rest of the plugin, are we?
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.
Now that you mention it, I actually don't remember seeing it around.
|
||
if ( 'core/template-part' === $block['blockName'] ) { | ||
if ( ! isset( $block['attrs']['postId'] ) ) { | ||
$template_part_file_path = |
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.
Do you think it would be worthwhile to extract this to get_template_part_file_path( $slug )
helper? It might help with readability here due to nesting, and I imagine we might need this resolution logic elsewhere. It can be abstracted further to include both templates and template parts if needed, since it seems that they follow the same pattern?
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 demo block template code is temporary until we have a proper block-based theme; this will flatten once we can remove it.
It would be nice if these were shown contextually depending on the currently selected template. After a template is selected these could be expanded below it, somewhat similar to block navigation. |
Sure. If I create a new template and add template parts with no content to it (it can be reproduced with post content and post title placeholders too), it shows up fine in the template switcher. After clicking on |
I think that was mocked up before, but it was discarded because you would be duplicating the entry point to focusing on a template part in all the templates that use it, and it would be harder to grasp all your template parts at once.
Yes, that's expected. The template will refresh, and the placeholder will remain, but there is no actual template part associated with the placeholder yet; it's just that, a placeholder for a future one. |
Size Change: +5.14 kB (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
"@wordpress/notices": "file:../notices" | ||
"@wordpress/notices": "file:../notices", | ||
"file-saver": "^2.0.2", | ||
"jszip": "^3.2.2" |
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.
Is this even used? It drew my attention because it's a 26kb+ dependency, but I don't see that we import it anywhere.
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 was used to export a theme in a zip file, but that functionality was removed. I think we can keep it as it will soon be introduced again.
Follows #19081
Description
This PR continues the
edit-site
work by loading all available templates and allowing you to switch between them.How has this been tested?
It was verified that all available templates are now shown in a dropdown in the editor sidebar and that it is possible to change between them, edit single or multiple ones, save single or multiple ones, and see the changes on the front end.
Screenshots
https://youtu.be/__VN9VNtT0E
Types of Changes
New Feature: The site editor now lets you edit all available templates.
Checklist: