Update the archive templates to use Products block #8308
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: +38 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
In 001685f, I reverted changes made to some archive templates that are fallback to |
dinhtungdu
added
focus: FSE
woocommercebot
requested review from
a team and
tjcafferkey
and removed request for
a team
patterns/product-search-form.php
Outdated
| * Inserter: no | ||
| */ | ||
| ?> | ||
| <!-- wp:search {"label":"<?php echo esc_html_x( 'Search', 'label', 'woo-gutenberg-products-block' ); ?>","showLabel":false,"placeholder":"<?php echo esc_attr_x( 'Search products…', 'placeholder for search field', 'woo-gutenberg-products-block' ); ?>","buttonText":"<?php esc_attr_e( 'Search', 'woo-gutenberg-products-block' ); ?>","query":{"post_type":"product"}} /--> |
There was a problem hiding this comment.
Considering "showLabel":false, you can skip providing a label:
| <!-- wp:search {"label":"<?php echo esc_html_x( 'Search', 'label', 'woo-gutenberg-products-block' ); ?>","showLabel":false,"placeholder":"<?php echo esc_attr_x( 'Search products…', 'placeholder for search field', 'woo-gutenberg-products-block' ); ?>","buttonText":"<?php esc_attr_e( 'Search', 'woo-gutenberg-products-block' ); ?>","query":{"post_type":"product"}} /--> | |
| <!-- wp:search {"showLabel":false,"placeholder":"<?php echo esc_attr_x( 'Search products…', 'placeholder for search field', 'woo-gutenberg-products-block' ); ?>","buttonText":"<?php esc_attr_e( 'Search', 'woo-gutenberg-products-block' ); ?>","query":{"post_type":"product"}} /--> |
| <div class="wp-block-query alignwide"> | ||
| <!-- wp:query-title {"type":"archive","showPrefix":false} /--> | ||
|
|
||
| <!-- wp:term-description /--> |
There was a problem hiding this comment.
For the Product Catalog template there will be no term, or term-description however it is displayed in the Site Editor. Is it possible to conditionally render this if the user is viewing the templates:
- Products by Category
- Products by Attribute
- Products by Tag (which by the way is not displaying the blockified version at this point, is this correct?)
There was a problem hiding this comment.
Hmm, good catch.
Is it possible to conditionally render this if the user is viewing the templates:
Currently, all archive templates use the same archive-product.html template (see #7712). I think we can do one of:
- Create a new archive template with description, fallback all other templates to use the one with description instead of the
archive-product.html. - Chime in the template parsing process to remove the description block for Product Catalog template.
- Remove the description block.
@albarin @tjcafferkey what do you think is the best direction here?
There was a problem hiding this comment.
Chime in the template parsing process to remove the description block for Product Catalog template.
This feels a little bit hacky and we could end up setting a precedent for other people to do the same thing for other template scenarios which might not be ideal. I'm open to people disagreeing though.
Create a new archive template with description, fallback all other templates to use the one with description instead of the archive-product.html.
This feels like a better option but comes with the overhead of maintaining two templates, would that be a problem?
There was a problem hiding this comment.
You read my mind Tom! If I have to choose one now, that'd be the second direction. But I'm not happy with either of them.
There was a problem hiding this comment.
I missed this block in template conversion PR. I'll include it there as well for Products by Category, Products by Attribute and Products by Tag
There was a problem hiding this comment.
Create a new archive template with description, fallback all other templates to use the one with description instead of the archive-product.html.
This feels like a better option but comes with the overhead of maintaining two templates, would that be a problem?
Not completely sure, but I think this would have more implications for the whole fallback system in place. Currently, if you make a modification to the Product Catalog template you should see it on the other three (cat, tag, attr). If we have a separate template with the description I think we'd loose this 🤔
Currently, all archive templates use the same archive-product.html template (see #7712). I think we can do one of:
- Create a new archive template with description, fallback all other templates to use the one with description instead of the archive-product.html.
- Chime in the template parsing process to remove the description block for Product Catalog template.
- Remove the description block.
I'll throw another option there, maybe unpopular 😬 what about just keeping the description block in there?
There was a problem hiding this comment.
I'll throw another option there, maybe unpopular 😬 what about just keeping the description block in there?
@albarin Aha! It's a legit one. Let's consider it also.
There was a problem hiding this comment.
I'll throw another option there, maybe unpopular 😬 what about just keeping the description block in there?
Definitely an option for now, the only problem is that it does not reflect what users will experience on the frontend. I wonder if its possible to put a note next to these blocks perhaps which explains "On the main Product Catalog page this information will not be visible" etc.
Just cc'ing @vivialice into this incase she has any ideas or would like to weigh in on one of the options.
Are you getting this issue? If yes, I notice the same, @albarin saved me by showing me the 7976 😄 |
@dinhtungdu this wasn't happening for that reason as none of the templates were customized. Trying to replicate this now it seems that there are some other issues and I'm not sure how many are related to my env/Gutenberg/this PR
|
@tjcafferkey and I paired yesterday and we can confirm the template is loading unreliable. This isn't an issue introduced by this PR. |
|
Testing this I was getting this error: Seems like the BlockPatterns.php expects all patterns to have a category. Should we include one in the two introduced here or maybe better to not make it required in the |
Nice catch! I think every pattern should have a category. Added in 9c2d701. Thank you! |
|
@tjcafferkey Because we're already injecting theme attributes into template content, I think it's fine to remove the term description block for the |
| @@ -198,6 +236,9 @@ public static function build_template_result_from_file( $template_file, $templat | |||
| $template->id = $template_is_from_theme ? $theme_name . '//' . $template_file->slug : self::PLUGIN_SLUG . '//' . $template_file->slug; | |||
| $template->theme = $template_is_from_theme ? $theme_name : self::PLUGIN_SLUG; | |||
| $template->content = self::inject_theme_attribute_in_content( $template_content ); | |||
| if ( 'archive-product' === $template_file->slug ) { | |||
There was a problem hiding this comment.
It would be great if we could leave a comment here just explaining why we're doing this.
There was a problem hiding this comment.
Thanks for sorting @dinhtungdu this LGTM! I have just left one comment but approving.
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
|
Failed tests are flaky. Merging with failed test. |
dinhtungdu
changed the title
dinhtungdu
changed the title
dinhtungdu
changed the title
dinhtungdu
added
the
type: enhancement
Fixes #8067
In this PR:
archive-product.htmlandproduct-search-results.htmltemplates to use the Products block instead of a placeholder.woocommerce_archive_descriptionhook now.archive-product.htmlremain unchanged in favor of Remove unused archive templates woocommerce#42481Accessibility
prefers-reduced-motionOther Checks
Screenshots
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog