-
Notifications
You must be signed in to change notification settings - Fork 219
Account for classic theme support of template parts #9780
Conversation
The release ZIP for this PR is accessible via:
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
Size Change: +1.04 kB (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
Yup! Please do :) |
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
Note this logic is currently not right - just a quick implementation for testing POC
540d07f
to
64b79d0
Compare
@Aljullu I will review this today for you |
src/BlockTemplatesController.php
Outdated
@@ -295,7 +295,7 @@ public function get_block_file_template( $template, $id, $template_type ) { | |||
* @return array | |||
*/ | |||
public function add_block_templates( $query_result, $query, $template_type ) { | |||
if ( ! BlockTemplateUtils::supports_block_templates() ) { | |||
if ( ! BlockTemplateUtils::supports_block_templates( 'wp_template_part' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd benefit from a comment here on why wp_template_part
for context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after taking another look, I think this should be $template_type
instead of wp_template_part
. Ie, if we are loading Templates, we want to check if the theme supports templates, not template parts.
I changed it in 1843a9a. Also, I think now the code is self-explanatory, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think you're right. I just checked the filter params to be sure it was always going to be one of the two (wp_template_part
/ wp_template
) and it looks to be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also re-tested and everything works as expected from what I can see unless the E2E tests pick up anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left a comment for a non-blocking change request but other than that this PR looks good and tests well so I think it's good 🚀
Classic themes are able to register support for template parts, so it'd be good to account for that with our registration of custom Woo Template Parts such as what is utilized for the Mini-Cart block.
Testing
User Facing Testing
User testing requires utilizing a classic WordPress theme which has implemented support for template parts (commonly via adding
add_theme_support( 'block-template-parts' );
to the themesfunctions.php
file via a callback on theafter_setup_theme
action hook:Once that is in place...
WooCommerce Visibility
Changelog