Skip to content
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

Merged
merged 36 commits into from
Feb 21, 2020

Conversation

epiqueras
Copy link
Contributor

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

Gutenberg Full Site Editing Experiment Site Editor Demo

Types of Changes

New Feature: The site editor now lets you edit all available templates.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@@ -69,6 +90,17 @@ export default function BlockEditor( { settings: _settings } ) {
onChange={ setContent }
>
<BlockEditorKeyboardShortcuts />
<Sidebar.TemplatesFill>
<TemplateSwitcher
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

@mtias
Copy link
Member

mtias commented Dec 14, 2019

Great to see this in conjunction with the entities save modal.

@jffng
Copy link
Contributor

jffng commented Dec 16, 2019

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 :)

  1. All the available templates show in the site editor dropdown, but not the template parts yet. Will this be a part of a separate PR, or am I missing something?

  2. The UI for adding templates from the site editor was confusing to me. Because of its location and persistence in the editor sidebar, it feels like it refers to the template that is currently active, rather than what it is: a persistent menu for adding new templates of a given name.

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.

https://cl.ly/6311242bf568

@jasmussen jasmussen mentioned this pull request Dec 17, 2019
6 tasks
@epiqueras
Copy link
Contributor Author

All the available templates show in the site editor dropdown, but not the template parts yet. Will this be a part of a separate PR, or am I missing something?

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.

The UI for adding templates from the site editor was confusing to me. Because of its location and persistence in the editor sidebar, it feels like it refers to the template that is currently active, rather than what it is: a persistent menu for adding new templates of a given name.

Yes, this is not meant to represent the final design. I just needed to expose a quick way of doing it.

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.

I think the idea is for it to be an option in the template switcher dropdown.

@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from 042999d to 2e171f9 Compare December 17, 2019 13:04
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from e654bb4 to 54553f2 Compare December 17, 2019 13:05
@epiqueras
Copy link
Contributor Author

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.

@jffng

#19203 (comment)

@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from 2e171f9 to f52a2f3 Compare January 8, 2020 01:44
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from 3222818 to 1d17971 Compare January 8, 2020 01:48
@epiqueras epiqueras requested a review from ellatrix as a January 8, 2020 13:06
@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from f52a2f3 to 7f8488c Compare January 9, 2020 19:51
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from e10f411 to e0e14bd Compare January 9, 2020 19:52
@@ -1,3 +1,4 @@
<!-- wp:template-part {"slug":"header","theme":"twentytwenty"} /-->
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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 and user 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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@epiqueras epiqueras changed the base branch from add/edit-site-template-loading to master January 16, 2020 15:05
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from 0335169 to 92a57cb Compare January 16, 2020 15:08
@youknowriad
Copy link
Contributor

I'm using the "demo templates", I've got two errors:

  • When switching from "Front Page" to "index", I have a lot of blocks broken with a JS error about "tinymce".
  • When adding a new template from the dropdown (I named it "post"), I got an empty screen.

$_wp_current_template_content = null;
$_wp_current_template_hierarchy = null;
$_wp_current_template_part_ids = null;
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from 885f6b7 to 8d4cbcb Compare February 18, 2020 13:59
@epiqueras
Copy link
Contributor Author

I've rebased the PR and extracted the exporter.

),
[ settings.templateIds, settings.templateId, setActiveTemplateId ]
) }
</URLPopover.LinkViewer.Fill>
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@youknowriad youknowriad Feb 21, 2020

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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 } ), [
Copy link
Contributor

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...?

Copy link
Contributor Author

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.

Copy link
Contributor

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 }
Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@vindl
Copy link
Member

vindl commented Feb 20, 2020

A note for future reviewers: the template switcher is no longer in the sidebar as indicated in the video linked in PR summary. It's now located in the editor header:

Screenshot 2020-02-20 at 21 20 10

@vindl
Copy link
Member

vindl commented Feb 20, 2020

Nice work @epiqueras! A couple of things I noticed while testing:

  1. Not sure if it's intended, but the newly created template parts are not showing in the template switcher even if I click on Update Design and save them explicitly. The show up only after a full page refresh.
  2. Empty template parts are not be saved (e.g. if I want to quickly leave some placeholders in page template)
  3. Creating a new template part requires a theme slug to be entered - I'd expect this to default to the current theme, but wondering if there are reasons for not doing that?

@epiqueras
Copy link
Contributor Author

Thanks for taking a look!

Not sure if it's intended, but the newly created template parts are not showing in the template switcher even if I click on Update Design and save them explicitly. The show up only after a full page refresh.

Yeah, that needs to be implemented. Probably with a REST API, I didn't want to explode this PR with.

Empty template parts are not be saved (e.g. if I want to quickly leave some placeholders in page template)

I can save placeholders with no problem. Can you provide more info?

Creating a new template part requires a theme slug to be entered - I'd expect this to default to the current theme, but wondering if there are reasons for not doing that?

It's not implemented yet. It should be part of the template part block redesign.

Copy link
Member

@vindl vindl left a 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,
Copy link
Member

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)?

Copy link
Contributor Author

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(
Copy link
Member

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:

$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' );

Copy link
Contributor Author

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 );
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Copy link
Contributor Author

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?

Copy link
Member

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 =
Copy link
Member

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?

Copy link
Contributor Author

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.

@vindl
Copy link
Member

vindl commented Feb 20, 2020

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.

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.

@vindl
Copy link
Member

vindl commented Feb 20, 2020

I can save placeholders with no problem. Can you provide more info?

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 Update Design I only get the option to save the template, and not the added placeholder template parts. After updating and a refresh the newly created template and template parts down show up in the template switcher.

@epiqueras
Copy link
Contributor Author

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.

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.

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 Update Design I only get the option to save the template, and not the added placeholder template parts. After updating and a refresh the newly created template and template parts down show up in the template switcher.

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.

@github-actions
Copy link

Size Change: +5.14 kB (0%)

Total Size: 864 kB

Filename Size Change
build/annotations/index.js 3.43 kB -3 B (0%)
build/api-fetch/index.js 3.39 kB +1 B
build/autop/index.js 2.58 kB -1 B
build/block-editor/index.js 104 kB +134 B (0%)
build/block-library/editor-rtl.css 7.67 kB +27 B (0%)
build/block-library/editor.css 7.67 kB +29 B (0%)
build/block-library/index.js 114 kB +2.25 kB (1%)
build/block-serialization-default-parser/index.js 1.65 kB -1 B
build/blocks/index.js 57.6 kB +4 B (0%)
build/components/index.js 190 kB +83 B (0%)
build/core-data/index.js 10.5 kB +30 B (0%)
build/data-controls/index.js 1.04 kB -3 B (0%)
build/data/index.js 8.22 kB -4 B (0%)
build/date/index.js 5.36 kB +1 B
build/dom-ready/index.js 569 B +1 B
build/edit-post/index.js 90.7 kB +142 B (0%)
build/edit-site/index.js 4.58 kB +1.87 kB (40%) 🚨
build/edit-site/style-rtl.css 2.77 kB +149 B (5%) 🔍
build/edit-site/style.css 2.76 kB +147 B (5%) 🔍
build/edit-widgets/index.js 4.36 kB +3 B (0%)
build/editor/index.js 45.1 kB -23 B (0%)
build/element/index.js 4.45 kB -1 B
build/format-library/index.js 7.6 kB -2 B (0%)
build/hooks/index.js 1.92 kB -1 B
build/html-entities/index.js 621 B -1 B
build/i18n/index.js 3.45 kB -1 B
build/keyboard-shortcuts/index.js 2.3 kB -6 B (0%)
build/keycodes/index.js 1.68 kB -1 B
build/media-utils/index.js 4.85 kB +12 B (0%)
build/notices/index.js 1.57 kB -1 B
build/nux/index.js 3.02 kB +3 B (0%)
build/plugins/index.js 2.54 kB +302 B (11%) ⚠️
build/primitives/index.js 1.49 kB -2 B (0%)
build/redux-routine/index.js 2.84 kB -3 B (0%)
build/rich-text/index.js 14.3 kB -1 B
build/token-list/index.js 1.27 kB +1 B
build/url/index.js 4 kB +1 B
build/viewport/index.js 1.61 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 9.78 kB 0 B
build/block-editor/style.css 9.77 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 16.1 kB 0 B
build/components/style.css 16 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/style-rtl.css 8.69 kB 0 B
build/edit-post/style.css 8.68 kB 0 B
build/edit-widgets/style-rtl.css 2.8 kB 0 B
build/edit-widgets/style.css 2.79 kB 0 B
build/editor/editor-styles-rtl.css 327 B 0 B
build/editor/editor-styles.css 328 B 0 B
build/editor/style-rtl.css 4.13 kB 0 B
build/editor/style.css 4.11 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 500 B 0 B
build/format-library/style.css 501 B 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 215 B 0 B
build/list-reusable-blocks/style.css 216 B 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/priority-queue/index.js 878 B 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@epiqueras epiqueras merged commit 320f460 into master Feb 21, 2020
@epiqueras epiqueras deleted the add/edit-site-multiple-template-loading branch February 21, 2020 01:23
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.6 Feb 26, 2020
"@wordpress/notices": "file:../notices"
"@wordpress/notices": "file:../notices",
"file-saver": "^2.0.2",
"jszip": "^3.2.2"
Copy link
Member

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants