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
Block Hooks: Run block hooks on all block themes #45581
Block Hooks: Run block hooks on all block themes #45581
Conversation
Test Results SummaryCommit SHA: 03d087c
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Hi @imanish003, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Nice! Since we have the filter(s) available for patterns and themes, I wonder if we could start promoting this via developer.woo.com to encourage authors of block themes to test WooCommerce with their themes (and patterns). Instructions can be provided to exclude problematic patterns/themes in the post/docs? |
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.
Hey @tjcafferkey, Great work & thanks for adding block hook support for all themes 🚀
I've tested the changes across additional themes, and everything is working as anticipated. While I've observed a few minor issues, I believe most of them are within acceptable bounds. However, I wanted to highlight them to ensure they align with our expectations. I am pre-approving the PR as I don’t consider them as blockers 🙂
- I noticed that the ‘Checkout header’ template parts doesn't have a mini-cart block in all themes. I believe that's expected, but I wanted to raise it here in case it's not.
Here is what I noticed with themes & most themes have common observations:
YITH Wonder
- Not all header template parts have mini-cart blocks, as you can see in the screenshot below. I believe this is expected, but I wanted to confirm with you.
Raft
- As you can see in the screenshot below, There are two mini-carts. Also, bottom one's placement isn't great. Is that expected?
Blockbase
- Having a breadcrumb in the middle feels somewhat off, though it's not a major issue. Ideally, it would be positioned alongside a mini-cart, preferably to the left of it. Your thoughts?
- The placement of the mini-cart could be improved.
@@ -38,18 +38,9 @@ public function register_hooked_block( $hooked_blocks, $position, $anchor_block, | |||
* | |||
* @since $VID:$ | |||
*/ | |||
$pattern_exclude_list = apply_filters( 'woocommerce_hooked_blocks_pattern_exclude_list', array( 'twentytwentytwo/header-centered-logo', 'twentytwentytwo/header-stacked' ) ); | |||
$pattern_exclude_list = apply_filters( 'woocommerce_hooked_blocks_pattern_exclude_list', array( 'twentytwentytwo/header-centered-logo', 'twentytwentytwo/header-stacked', 'blockbase/header-centered' ) ); |
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.
Nit: I am wondering if we should directly contribute to blockbase
theme instead of excluding it here as Automattic owns it. Shouldn't themes leverage the woocommerce_hooked_blocks_pattern_exclude_list
hook to exclude patterns instead of incorporating these exclusions within the WooCommerce core? 🧐
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 should do both. Exclude it for now > Contribute > Include it back in. I'm leaving it excluded for now because if we want to fix this issue it'll fall quite low on the priority list for now and I'm unsure on timescales.
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.
Makes sense to me 🤝
@nerrad This PR removes the filter for As for patterns we can put some documentation together for this, I agree it could be very useful. I'll hold off merging this PR until you see this incase you foresee any problems with removing the filter or want us to replace it with a filter to exclude themes? |
I think replacing it with an exclude filter could be useful. It also provides an escape hatch for hosts/agencies that want finer control over auto-added blocks. With that said, is there a way in the WP core implementation to achieve similar functionality? |
@nerrad they could add a filter (priority 999) to the This would be a better way to achieve the same thing I suppose because it keeps the 3PDs closer to the Core API which means less maintainability for Woo (we could explain this method in our post). It would cause less confusion too, when we explain we have our own filter for essentially doing the same thing. |
Yup agreed. I'd prefer keeping our API surface small and this is something that could be surfaced in documentation if needed. |
For future reference, here's a method to remove the auto-insertion of mini-cart block if it's not desired:
|
This PR removes the guard for inserting WooCommerce's hooked blocks on approved themes only, if accepted this will allow us to hook blocks into all block themes.
Themes that were already approved:
Additional themes I've tested
As you can see from the above we are already inserting out hooked blocks into the most popular themes which were the already approved ones. Based on my additional testing of themes that would be impacted after this change they all seem to handle the insertion of the Mini Cart well.
That said though there are a few cases (see screenshot below of the Attar themes default header) where we can continue to make improvements similar to how we did with TT4, TT3, TT2
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes # .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Changelog entry
Significance
Type
Message Block Hooks: Run block hooks on all block themes instead of approved themes only.
Comment