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

Templates: Add demo templates directory and resolution logic. #18554

Merged
merged 7 commits into from Nov 28, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 15, 2019

Closes #18538

Description

This PR adds a local fallback directory (lib/demo-block-templates/) to the theme block template resolution process. This way, we can bundle initial demo templates with Gutenberg.

This PR adds an index demo block template and that allows us to remove the filter that blocked users from deleting their own index templates.

How has this been tested?

It was verified that block template resolution in the full site editing experiment now looks at the fallback demo directory as a last resort when resolving templates. E.g. deleting all template CPT posts should make your post render in lib/demo-block-templates/index.html, assuming your theme doesn't provide any matching, higher or equal priority templates.

Types of Changes

New Feature: The full site editing experiment can now ship demo block 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. .
if ( is_child_theme() ) {
$block_template_files = array_merge( $block_template_files, glob( get_template_directory() . '/block-templates/*.html', 1 ) );
}
$block_template_files = array_merge( $block_template_files, glob( dirname( __FILE__ ) . '/demo-block-templates/*.html', 1 ) );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 16, 2019

Contributor

My concern here is that the "demo" fallback shouldn't be bundled in the "template loader". This code will ultimately land in Core and ideally, we should keep the default behavior and the demo enabling/disabling separate.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 16, 2019

Author Contributor

Maybe "demo" is not the right term then.

I imagine Core shipping with a demo/fallback index template like this. So that the site doesn't break when no templates are available and to support template-less themes that only modify styles.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 16, 2019

Contributor

Well these will be the default themes IMO, no real need for a built-in fallback.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 16, 2019

Author Contributor

Then can we just remove this line later?

It doesn't hurt, even if the folder is not present.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 18, 2019

I still get this when trying to test that branch. What am I doing wrong?

Capture d’écran 2019-11-18 à 1 57 33 PM

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 18, 2019

https://www.php.net/manual/en/function.glob.php

Note:

On some systems it is impossible to distinguish between empty match and an error.

It wasn't taking that into account. It should work now, with the latest commit.

@epiqueras epiqueras requested review from ellatrix and talldan as code owners Nov 18, 2019
Copy link
Member

felixarntz left a comment

I'm quite wary of adjusting the template detection logic that much for just a demo, especially regarding intertwining the demo with potential real themes that support block templates. A real theme could work very differently from the demo templates, so we shouldn't mix those. It's a different thing with a theme and its child theme since the latter is clearly built on top of it.

I think we should simplify this and only use demo templates if the current theme does not have block templates at all. That should be the only use-case anyway, which is important today, but not as much in the long term once themes start adoption.

lib/experiments-page.php Show resolved Hide resolved
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 19, 2019

The reason I suggested a second flag is this:

  • We need to build a fallback mechanism for the template resolution to fallback to php templates in order for themes to opt-in slowly into block-editor templates. This means theme authors need to enable Full Site Editing but their site shouldn't break, their php templates should continue to work unless they have a higher-priority block template
  • I say this "demo content" as a "demo FSE theme" basically, but since it's not possible to embed a theme in a plugin, a flag to enable/disable that theme is the way to go IMO.
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 19, 2019

@felixarntz also note that this is not intended to be shipped outside of the experiment or dev plugin.

@felixarntz

This comment has been minimized.

Copy link
Member

felixarntz commented Nov 22, 2019

@youknowriad @epiqueras In order to make their actual existing PHP templates work unless they also have block templates, we would need to do some weird things like e.g. taking all the markup from these templates and putting it into a single Custom HTML block or so - it would almost be like a Classic Editor block today, but for a "classic theme". But I don't see much value in this.

I understand the reasoning behind the demo FSE theme, that makes sense to me. However, even if we don't ship this demo, we are now heavily adjusting template detection logic in this PR, just for this. I'd prefer to limit the adjustments to a minimum, specifically, as I mentioned before, I don't think we should mix the actual block template detection logic too much with the fallback mechanism. Why not do it like this:

  • If the current theme has /block-templates/index, we can safely assume this theme supports FSE, so here we would not use the demo FSE theme at all.
  • If the current theme does not have /block-templates/index and the flag for the demo is set, we only rely on the templates from the demo.

Alternatively, we could also do it fully depending on the demo flag, i.e. if the demo FSE theme is enabled, always use those templates instead of the current theme one (that's where the individual experiment would indeed come in handy).

Most importantly, I don't think we should intertwine block template detection logic of the actual theme with the demo theme. Imagine on a WP site today some templates would look like Twenty Twenty, while others would look like Twenty Nineteen - that would be super weird, and the same would be a likely result of intertwining the logic as currently done, mixing real theme and demo theme.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 22, 2019

Most importantly, I don't think we should intertwine block template detection logic of the actual theme with the demo theme

I completely agree with this and it's one of the reasons I suggested a separate flag. Ideally, it's a separate theme but we can't bundle a theme in a plugin.

In order to make their actual existing PHP templates work unless they also have block templates, we would need to do some weird things like e.g. taking all the markup from these templates and putting it into a single Custom HTML block or so - it would almost be like a Classic Editor block today, but for a "classic theme". But I don't see much value in this.

This I'm not certain about it, the idea is: if there's no block-template defined, just fallback to the existing rendering mechanism that uses the php templates. (this is a separate issue/PR though)

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 22, 2019

In order to make their actual existing PHP templates work unless they also have block templates, we would need to do some weird things like e.g. taking all the markup from these templates and putting it into a single Custom HTML block or so - it would almost be like a Classic Editor block today, but for a "classic theme". But I don't see much value in this.

I don't see why that would be necessary. I think we can glob them in gutenberg_find_template and return them instead of the template canvas when it makes sense to do so.

we are now heavily adjusting template detection logic in this PR, just for this.

GitHub makes it look big, because I removed a level of indentation, but the change is really small and simple:

+ if ( gutenberg_is_experiment_enabled( 'gutenberg-full-site-editing-demo' ) ) {
+ $block_template_files = array_merge( $block_template_files, glob( dirname( __FILE__ ) . '/demo-block-templates/*.html', 1 ) ?: array() );
+ }
- $theme_block_template_priority < $slug_priorities[ $current_template_post->post_name ]
+ ( empty( $current_template_post ) || $theme_block_template_priority < $slug_priorities[ $current_template_post->post_name ] )

All we're doing is adding another fallback level to the resolution and adding an if guard for when there is no current template post.

Most importantly, I don't think we should intertwine block template detection logic of the actual theme with the demo theme. Imagine on a WP site today some templates would look like Twenty Twenty, while others would look like Twenty Nineteen - that would be super weird, and the same would be a likely result of intertwining the logic as currently done, mixing real theme and demo theme.

I agree with this, but that is not how this demo content will be used. The demo content will mostly be used to provide a nice index that showcases what FSE can do. If I implement your heuristic for deciding when to activate it, dev/test users that want to see the demo would have to go activate a legacy theme and trash all their current templates. We could make it so the heuristic only considers theme templates, but then it is even more confusing and harder to explain.

In any case, implementing any of these is super easy on top of the changes in this PR and I don't think this should block it. It's a development focused feature that we can always tweak later if we see people are getting confused.

Copy link
Member

felixarntz left a comment

The demo content will mostly be used to provide a nice index that showcases what FSE can do. If I implement your heuristic for deciding when to activate it, dev/test users that want to see the demo would have to go activate a legacy theme and trash all their current templates.

I'm still a bit hesitant, but let's try it out. I think using it as a showcase should be focused on giving end users and developers an idea of how this functionality will work. Whenever someone would start developing a theme supporting FSE, they have already taken the decision to support it. I still feel like, at that point, falling back to templates they haven't actually provided can cause confusion, and potentially even give a false sense of already having implemented those templates (provided by the demo).

I'm okay to move forward with this and see how people respond to it.


return $caps;
}
add_filter( 'map_meta_cap', 'gutenberg_prevent_index_template_deletion', 10, 4 );

This comment has been minimized.

Copy link
@felixarntz

felixarntz Nov 28, 2019

Member

Good point removing this one. Now that we have the compatibility with block templates provided by the theme, deleting the index template means basically resetting it (to the theme template), which is something that should be allowed.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 28, 2019

Author Contributor

Exactly 😄

@epiqueras epiqueras force-pushed the add/demo-block-template branch from 5afb0fc to e5c08c7 Nov 28, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 28, 2019

I still feel like, at that point, falling back to templates they haven't actually provided can cause confusion, and potentially even give a false sense of already having implemented those templates (provided by the demo).

Agreed, that's why we made it a separate experiment flag.

I'm okay to move forward with this and see how people respond to it.

👍

@epiqueras epiqueras force-pushed the add/demo-block-template branch 2 times, most recently from bd47ef8 to b1769e2 Nov 28, 2019
@epiqueras epiqueras force-pushed the add/demo-block-template branch from b1769e2 to d1c897e Nov 28, 2019
@epiqueras epiqueras merged commit b066ac9 into master Nov 28, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras added this to Done in Phase 2 via automation Nov 28, 2019
@epiqueras epiqueras deleted the add/demo-block-template branch Nov 28, 2019
@scruffian scruffian mentioned this pull request Nov 28, 2019
1 of 6 tasks complete
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019
@epiqueras epiqueras mentioned this pull request Dec 13, 2019
0 of 9 tasks complete
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.