Add an 'area' term for Template Parts. #28410
Conversation
Addison-Stavlo
changed the title
|
Size Change: -190 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
Addison-Stavlo
requested review from
mattwiebe,
Copons,
vindl,
david-szabo97,
jeyip and
youknowriad
Addison-Stavlo
added
[Feature] Full Site Editing
[Status] In Progress
Addison-Stavlo
changed the title
Addison-Stavlo
changed the title
Addison-Stavlo
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
Addison-Stavlo
removed
the
[Status] In Progress
TestingBehaviorFollowed the instructions and everything is working as expected. I'll spend more time smoke testing the normal template part flows tomorrow.
BrowsersSmoke tested the normal template flows in other browsers
Did not test IE11 because of an existing issue In testing I identified a bug unrelated to this PR that already exist on master. I'll leave notes here and make an issue so other testers don't need to rehash any investigation. |
|
I wonder if "section" is the right term for this. 🤔 Somehow my brain is stuck thinking that "location" might be more appropriate, as a kind of callout to Neither sound perfect, though. Maybe we already have a WordPress-y term that fits this use-case? |
@Copons - Yeah, I'm not certain what the best name is either. We were discussing this in the above comments (#28410 (comment)), which I 'unresolved' for now if we want to keep discussing better alternatives. |
|
The closest prior art in WP for this is Widget Areas, itself an attempt at better naming for Sidebars that were no longer always on the side, or a bar, for that matter. So I'd suggest The main reason I do like Otherwise zone and region are nice standouts from my trip to the thesaurus. |
|
@mattwiebe - That makes sense to me! Updated now so we register the term as |
|
Updated per feedback:
|
| $this->assertEquals( 'header', $template_part->title ); | ||
| $this->assertEquals( '', $template_part->description ); | ||
| $this->assertEquals( 'wp_template_part', $template_part->type ); | ||
| // TODO - update null to 'header' once tt1-blocks theme.json updated for template part section info. |
There was a problem hiding this comment.
Feels weird that we are depending on the TT1B theme for unit tests. Isn't there a way to create a theme fixture and use that for unit testing? (Not a blocker, but it would be a great follow up)
There was a problem hiding this comment.
Isn't there a way to create a theme fixture and use that for unit testing?
🤔 Im not sure the best way to go about that. It seems like it would need to be installed and activate-able like a regular theme, with the same file structures. And at that point maybe using TT1B just makes the most sense (as opposed to creating and maintaining a separate one just for testing) since it is the most kept up-to-date block-based theme that we are developing FSE around currently. I wonder if @youknowriad has given this any thought previously?
There was a problem hiding this comment.
Riad is AFK until next week. Personally, I'm leaning towards using TT1 for the reasons Addison presents.
There was a problem hiding this comment.
It makes more sense for E2E tests. I don't think it's reasonable for unit tests. We opened an issue to discuss it: #28692 Would love to hear your thoughts. It's not a blocker, just a thought 😄
👍 Sounds good. Updated! ✅ |
|
Tested according to the testing instructions and smoke tested the editor. Everything works fine! ✅ |
TestingBehaviorFollowed the instructions and everything is working as expected with the new area changes.
|
|
Oh! My mistake. I am seeing something potentially odd. +1 whenever we resolve this conversation. I added a new template part to the tt1-blocks index template in the site editor. The area value seems strange
|
The typo is here: Wonder why PHP evaluates an undefined constant as a string literal, though... 😮 |
Oh good catch! I assumed there would be typos from going through and renaming everything but thought I had caught them all. Should be fixed now.
TIL 🤷♀️ |
| } | ||
|
|
||
| if ( isset( $theme_data[ $template_info['slug'] ]['area'] ) ) { | ||
| $template_info['area'] = gutenberg_filter_template_part_area_type( $theme_data[ $template_info['slug'] ]['area'] ); |
There was a problem hiding this comment.
There's some inconsistency in different parts of the code base for instance in this function namegutenberg_filter_template_part_area_type we refer to the "area" as "type" which can be confusing especially since we already have a "type" property on the template and template area objects.
There was a problem hiding this comment.
Id say here we refer to 'area' as 'area', and 'type' more refers to the different allowed values for that area term. Perhaps gutenberg_filter_template_part_area_value would make more sense?
There was a problem hiding this comment.
I like value better, but why not just gutenberg_filter_template_part_area ?
There was a problem hiding this comment.
that works too, names are hard 😅
|
Can you point me to the documentation that explains how to use Can |
|
Hi @gziolo!
The example in the PR description should work, but here is how it is set up in tt1-blocks (https://github.com/WordPress/theme-experiments/blob/1a4128f7c2c741e9c14b4c2a6bffd9c5b06e77e2/tt1-blocks/experimental-theme.json#L2-L9).
I don't think translation makes sense here, as only specific values are allowed and these are all "under the hood". When we surface an allowed value in the editor as an option, that is translated.
I believe the thought behind that unit test is to ensure that if the value is registered as something that is not supported, that it gets turned into the 'uncategorized' value. So "some area" is just an arbitrary unsupported string that should come get mapped to 'uncategorized'. |
|
editing comments seems a bit broken at the moment:
I think we need to add some proper documentation for this. Do you know where the best place for that would be? |
A good first step would be to add it to the current theme.json spec document (source). |
👍 - Thanks! That sounds like a good place to start - #30118 |
| @@ -237,6 +280,15 @@ function gutenberg_get_block_templates( $query = array(), $template_type = 'wp_t | |||
| ), | |||
| ); | |||
|
|
|||
| if ( 'wp_template_part' === $template_type && isset( $query['area'] ) ) { | |||
There was a problem hiding this comment.
'area' is a new key under $query and isn't documented in the gutenberg_get_block_templates docblock. @Addison-Stavlo, could you follow up with that quick addition? 🙏
There was a problem hiding this comment.
Certainly! My apologies for the oversight there earlier - #32746
Description
This PR explores adding an 'area' (im open to better naming suggestions) term to aid in being able to classify and request all template parts corresponding to a specific type ('header', 'footer', etc.) - something like this will be necessary to enable items discussed in #27337
Current state of this PR:
wp_template_part_area.templateParts[ $slug ][ 'area' ]. For example, to define the 'area' of a template part whose file is named "file-name.html" the json would look like:How has this been tested?
Many of the following steps are covered in the added php unit tests. To test manually:
1 - Verify the term is added on singular requests and persists across saving.
entityRecordhere) and verify the header's "area" key is present and has the expected value "header".true), and verify the term key/value is still present as expected.2 - Verify querying by the term works for both non-custom and custom template parts.
area: "header"and log the result. One way to do this is to add a query to this request and log the result. This request can be triggered by pressing the "down" chevron in a template part block's toolbar (to the right of the renaming input).true) and verify it is the only result.3 - Verify this does not work for unsupported types.
headerto typefoobar.uncategorized.4 - Smoke test the normal template part flows.
Screenshots
Types of changes
Checklist: