Full Site Editing: Overwrite theme attributes in file based templates #28207
Conversation
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
| @@ -122,7 +122,6 @@ function _inject_theme_attribute_in_content( $template_content, $theme ) { | |||
| foreach ( $template_blocks as $key => $block ) { | |||
| if ( | |||
| 'core/template-part' === $block['blockName'] && | |||
| ! isset( $block['attrs']['theme'] ) && | |||
There was a problem hiding this comment.
Note:
If we remove this line of code, we will always ignore theme attributes provided by file based templates and replace it with the current theme's stylesheet.
There was a problem hiding this comment.
How is this different to what #28131 does? It appears this forces the block attribute to always have the same value. Isn't that the same as removing support altogether?
There was a problem hiding this comment.
How is this different to what #28131 does?
From what I understand about #28131, it attempts to do away with the theme attribute entirely. (Definitely let me know if I'm misreading!)
With these code changes, the theme attribute itself is still being utilized. We're just:
- stripping theme attributes provided by file-based templates
- replacing the old theme attribute with a new one ( the current theme's stylesheet )
We're still relying on theme attributes to query for and to load templates/ template parts.
There was a problem hiding this comment.
the theme attribute itself is still being utilized.
We're still relying on theme attributes
Is it? Are you? If I am running a theme my-theme, then any of these will end up resolving in the same way:
<!-- wp:template-part {"slug":"header","theme":"my-theme"} /-->
<!-- wp:template-part {"slug":"header"} /-->
<!-- wp:template-part {"slug":"header","theme":"I can put anything at all in here and it will resolve the same"} /-->
So, the question is, what effect does the attribute have?
There was a problem hiding this comment.
Thanks for the question! I'm trying to make sure I understand you correctly, so this answer is a little verbose. Just did some quick tests in my local environment to double check, but here's what I see when working with theme provided, file-based templates.
Without this PR applied:
// Loads as long as the theme we're referencing lives in the root of the theme directory (wp-content/themes)
<!-- wp:template-part {"slug":"header","theme":"my-theme"} /-->
// Loads properly because of the changes in #28088
<!-- wp:template-part {"slug":"header"} /--> //
// Does not load
<!-- wp:template-part {"slug":"header","theme":"I can put anything at all in here and it will resolve the same"} /-->
With this PR applied:
// Loads properly
<!-- wp:template-part {"slug":"header","theme":"my-theme"} /-->
// Loads properly
<!-- wp:template-part {"slug":"header"} /--> //
// Loads properly
<!-- wp:template-part {"slug":"header","theme":"I can put anything at all in here and it will resolve the same"} /-->
I believe you're trying to comment on what happens when we apply this PR, and you're saying that, regardless of what we use as a theme attribute, the file based template part still loads. It appears as if, when provided by theme files, the theme attribute no longer matters.
If this is what you mean, then yes, the intention of these code changes is to ignore any theme attributes defined in file-based templates. These changes, however, only affects file-based templates (see here), and wouldn't change how customized template theme attributes are handled (see here). That is to say, we wouldn't be stripping theme attributes from user-customized and saved templates.
In that sense, we're not removing support for block attributes.
There was a problem hiding this comment.
@jeyip Your explanation makes total sense and you did understand correctly, although I'm not sure "stripping" is the correct description of what this patch does. It's more a case of "overwriting" or "overriding" the value of the attribute with the active theme.
You clarified this wouldn't apply to user-customised templates, but I've noticed that the present version of the master branch actually has no UI to vary the value from the active theme anyway. It can only be done through code view.
Furthermore, as I mentioned in this comment, it would be very difficult to build a useful UI for varying the theme value:
The problem with reinstating the theme reference into the endpoint is it would make it extremely difficult to list out the "active" template-parts (in the site editor, for example) without listing out every template-part from every theme.
In other words, by continuing support for the theme attribute, you're only providing immediate support for users who know how to use the editor's code view, and no one presently knows how any potential future UI will work or whether it will eventuate at all.
I guess it's up to you whether you're OK with this, but it seems to me like a dysfunctional way to run the project. Maybe you weren't aware of some of these points.
There was a problem hiding this comment.
although I'm not sure "stripping" is the correct description of what this patch does. It's more a case of "overwriting" or "overriding" the value of the attribute with the active theme.
Good point! I changed the PR title to reflect this.
You clarified this wouldn't apply to user-customised templates, but I've noticed that the present version of the master branch actually has no UI to vary the value from the active theme anyway. It can only be done through code view.
I think I follow. Currently, the theme attribute for user-customized template parts is set through the "code view" in the editor, and is always assigned the currently active theme.
Furthermore, as I mentioned in this comment, it would be very difficult to build a useful UI for varying the theme value:
I'm not sure I follow this. 🤔 Could you elaborate on why we'd want to vary the theme value? I think you're talking about giving users themselves the ability to assign theme attributes to the template part, but I'm not certain.
There was a problem hiding this comment.
Could you elaborate on why we'd want to vary the theme value?
Ostensibly, for requesting template parts from themes other than the active one.
jeyip
changed the title
jeyip
requested review from
mattwiebe,
Copons,
vindl,
Addison-Stavlo and
david-szabo97
jeyip
changed the title
There was a problem hiding this comment.
I'm not sure I agree with this change because it gives the wrong signal to the theme authors: It tells them you can do whatever you want in the theme attribute, It doesn't matter.
I wonder if a better approach would be to actually trigger an error if it's not empty (at least for the time being)
What if we could remap template parts in the block editor? We could show them as "Templart part couldn't be found" rather than an infinitely loading spinner. With a "Locate" button next to the error, serving the purpose of choosing an existing template part from a popover. I think this would be a good user experience. It reminds me of Photoshop or any video editing software - if a file is missing that's referenced in the project, you can "locate" them manually. If a font is missing in Photoshop, it warns you and you can address it. |
Oh defintely, I just pushed a similar fix for Reusable blocks lately, I wasn't aware it happened in template parts too. #28126 |
I think this would be fair if the documentation is updated to reflect that attribute is not intended for markup. Throwing the error when its non-empty may be appropriate as well. But all in all, there is nothing a theme developer can set a |
This sounds fine to me.
I think that the documentation should be updated regardless of what we decide here. |
| @@ -87,14 +87,6 @@ function test_inject_theme_attribute_in_content() { | |||
| ); | |||
| $this->assertEquals( $expected, $template_content ); | |||
|
|
|||
| // Does not inject theme when there is an existing theme attribute. | |||
| $content_with_existing_theme_attribute = '<!-- wp:template-part {"slug":"header","theme":"fake-theme","align":"full", "tagName":"header","className":"site-header"} /-->'; | |||
There was a problem hiding this comment.
Instead of deleting this would it make sense to update it and test the case where theme attribute is not provided?
|
Closing this for now, but we can reopen if a concrete use case for overwriting theme attributes from files arises |
Copons
deleted the
try/stripping_theme_attributes_from_file_based_templates
branch
Description
This ticket is a follow up to #28088.
To simplify and constrain the scope of full site editing work, there was a prior decision made to assume that when we serve template files, they're always from the currently active theme. See the discussion here. There were also discussions about removing the theme attribute from file-based template parts.
Following that logic, there were questions about just stripping the theme attribute in template files entirely, and replacing them with the currently active theme. We couldn't think of valid use cases otherwise, so this PR is a proposal to ignore theme attributes from file-based templates and to replace them with the current theme's stylesheet.
Note:
I left a note in the PR description of #28088. We were were planning to remove the theme attribute entirely from all file-based template parts (and I'd say it's still worthwhile to do so). These changes accomplish the same goal, but do so in a programmatic and scalable way.
How has this been tested?
Screenshots
Types of changes
Bug fix for Automattic/wp-calypso#48145
Checklist: