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

Open
wants to merge 34 commits into
base: master
from

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Dec 13, 2019

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. .
@epiqueras epiqueras added this to the Future milestone Dec 13, 2019
@epiqueras epiqueras self-assigned this Dec 13, 2019
@epiqueras epiqueras mentioned this pull request Dec 13, 2019
0 of 9 tasks complete
@@ -69,6 +90,17 @@ export default function BlockEditor( { settings: _settings } ) {
onChange={ setContent }
>
<BlockEditorKeyboardShortcuts />
<Sidebar.TemplatesFill>
<TemplateSwitcher

This comment has been minimized.

Copy link
@mtias

mtias Dec 14, 2019

Contributor

Can we move the template switcher to the editor header?

This comment has been minimized.

Copy link
@mtias

mtias Dec 14, 2019

Contributor

Actually both features, the "add template" as well.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 14, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@mtias

mtias Dec 14, 2019

Contributor

I think that's separate, we should still be able to use a dropdown menu from the existing components and list the current template.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 14, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

mtias commented Dec 14, 2019

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

@jffng

This comment has been minimized.

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
0 of 6 tasks complete
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 17, 2019

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 Dec 17, 2019
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from e654bb4 to 54553f2 Dec 17, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 18, 2019

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)

@karmatosed karmatosed added this to Inbox in Full site editing Dec 19, 2019
@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from 2e171f9 to f52a2f3 Jan 8, 2020
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from 3222818 to 1d17971 Jan 8, 2020
@epiqueras epiqueras requested a review from ellatrix as a code owner Jan 8, 2020
@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from f52a2f3 to 7f8488c Jan 9, 2020
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from e10f411 to e0e14bd Jan 9, 2020
@@ -1,3 +1,4 @@
<!-- wp:template-part {"slug":"header","theme":"twentytwenty"} /-->

This comment has been minimized.

Copy link
@mtias

mtias Jan 10, 2020

Contributor

Do we need the theme slug here? It'd be good if it could be implicit to the "current theme" as set in options.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 10, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@mtias

mtias Jan 10, 2020

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 10, 2020

Author Contributor

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.

@epiqueras epiqueras changed the base branch from add/edit-site-template-loading to master Jan 16, 2020
@epiqueras epiqueras force-pushed the add/edit-site-multiple-template-loading branch from 0335169 to 92a57cb Jan 16, 2020
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 22, 2020

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;
}

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 22, 2020

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 22, 2020

Author Contributor

Good idea. It might be useful for plugin developers as well.

*/
import getBaseThemeZip from './get-base-theme-zip';

export default function ThemeExporter( { ids, templatePartIds } ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 22, 2020

Contributor

This PR is doing too much IMO, can't we keep the exporter for a separate PR.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 22, 2020

Author Contributor

I thought it was ok because it's a stand-alone prototype and doesn't change public APIs.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 22, 2020

Contributor

The PR is too big and harder to test, review and land. It's not about APIs it's about landing small pieces iteratively.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 22, 2020

Author Contributor

I'll break this one up once it gets pseudo-approved, but I worry we are falling for a dogmatic requirement of keeping PRs small.

There is a higher up-front cost for review sure, but you save on the overhead of stacking PRs, and complex rebases every time a significant change happens on the master branch, or you finally get a review, and one of the base branches is merged.

In general, I agree that smaller PRs tend to be better. Still, there is a place for PRs like this when introducing an entirely new module that doesn't modify existing code, especially when the set of features introduced are almost all sequential. If they were things that could be merged parallelly, I would see value in a parallel approach. Otherwise, you're increasing the total person-hours to get the same code in with the same scrutiny, or less, if you consider different people could review the various sequential PRs without the best context of the previous and future changes.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 22, 2020

Contributor

I have to disagree, loading multiple entities, and exporting a theme seems like two complete different features to me and are better addressed in separate PRs. (no dogme here)

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 22, 2020

Author Contributor

I wasn't thinking about specific examples, but you can't test exporting a theme without loading the templates.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 22, 2020

This PR deserves more eyes, both design and tech. cc @aduth @jorgefilipecosta @jasmussen @MichaelArestad

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Jan 22, 2020

When switching from "Front Page" to "index", I have a lot of blocks broken with a JS error about "tinymce".

The location of tinymce on window changed recently, and blocks that rely on it broke. This is unrelated to this PR.

When adding a new template from the dropdown (I named it "post"), I got an empty screen.

Do you have a console message? I can't reproduce this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.