This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Add a compatibility layer to keep extensions continue working with Blockified Archive Templates #8172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dinhtungdu
added
focus: FSE
|
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: 0 B Total Size: 1.08 MB ℹ️ View Unchanged
|
woocommercebot
requested review from
a team and
danieldudzic
and removed request for
a team
dinhtungdu
changed the title
nerrad
reviewed
Jan 13, 2023
Aljullu
reviewed
Jan 16, 2023
There was a problem hiding this comment.
This looks great, @dinhtungdu! 👏 I left some comments inline but overall looking good.
| ), | ||
| ), | ||
| 'core/query-no-results' => array( | ||
| 'woocommerce_no_products_found' => array( |
There was a problem hiding this comment.
woocommerce_no_products_found seems to be triggered even if there are products in the loop:
There was a problem hiding this comment.
Yeah, I noticed the same issue.
There was a problem hiding this comment.
I fixed this issue by checking the block content before injecting the hooks.
|
Great work, Tung! |
nerrad
reviewed
Jan 17, 2023
Comment on lines
105
to
129
| if ( empty( $block_content ) ) { | ||
| return $block_content; | ||
| } |
There was a problem hiding this comment.
Will this mean the woocommerce_no_products_found won't fire?
There was a problem hiding this comment.
This makes the actions fire only when blocks return non-empty content. For the core/query-no-results case, when the query has products, the block content will be empty, then woocommerce_no_products_found shouldn't be fired.
But this will be false positive if we remove all inner blocks from core/query-no-results. We need a second thought on this 🤔 .
| * | ||
| * @param array $hook_data Hook data. | ||
| */ | ||
| return apply_filters( 'woocommerce_blocks_hooks_compatibility_data', $hook_data ); |
There was a problem hiding this comment.
I wonder if we should preserve the original hook data instead of allowing it to be overwritten by the filter? that would reduce the ability for extensions to muck things up accidentally? It'd also mean the shape of the data can be validated before passing on to callers.
I'm on the fence about whether we should just provide a registration mechanism for these to make it easier for extensions to register additional items (especially given it'd be easy to mess up the array). What has me thinking to stick with the filter is the likelihood of it getting used much.
Comment on lines
204
to
208
| /** | ||
| * Filter the hook data used to replace the default WooCommerce hooks. | ||
| * | ||
| * @param array $hook_data Hook data. | ||
| */ |
There was a problem hiding this comment.
Given the complexity of the hook data array, it'd be good to expand on the doc block a bit more about its shape.
|
In bccfa1a, I refactored the hook data structure to reduce the depth of the array as well as make it easier to manipulate the data inside the class. For the filter, I limited the data developer can pass to the filter to make it safer and easier to use. Instead of letting developer change the all hook data, now we only allow them to add new hooked functions to the supported hooks. Can you take another look at this @nerrad? Thank you. |
nerrad
reviewed
Jan 17, 2023
nerrad
reviewed
Jan 18, 2023
| return; | ||
| } | ||
|
|
||
| foreach ( $additional_hook_data as $data ) { |
There was a problem hiding this comment.
Should make sure $additional_data is an array before treating it as one here. Extensions could muck up the type so let's validate first.
There was a problem hiding this comment.
Should be fixed in 1cdcd5c.
nerrad
approved these changes
Jan 19, 2023
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
Co-authored-by: Darren Ethier <[email protected]>
Aljullu
force-pushed
the
add/archive-template-compatibility-layer
branch
from
1cdcd5c to
0765a73
Compare
|
I did a last round of testing and everything is looking good on my side. There are some e2e tests failing, but they are failing in |
nielslange
added
the
type: enhancement
17 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8065
This PR adds a new class that acts as the compatibility layer for blockified archive templates by:
The compatibility is building around Products block because loop is the main element of archive templates and hooks are placing inside and around the loop.
This PR skips these hooks:
woocommerce_show_page_titlewoocommerce_archive_descriptionwoocommerce_sidebarAccessibility
prefers-reduced-motionOther Checks
Testing
Automated Tests
User Facing Testing
composer dump-autoload.Inherit query from template.functions.phpfile, add this snippet, then reload the shop page on the front end to see if the test content is displayed at the expected position:Repeat for other hooks:
woocommerce_before_main_contentwoocommerce_after_main_contentwoocommerce_before_shop_loop_item_titlewoocommerce_shop_loop_item_titlewoocommerce_after_shop_loop_item_titlewoocommerce_before_shop_loop_itemwoocommerce_after_shop_loop_itemwoocommerce_before_shop_loopwoocommerce_after_shop_loopwoocommerce_no_products_foundWooCommerce Visibility
Performance Impact
Changelog