Wrap the Single Product Template in a div with the product class #8364
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: 0 B Total Size: 1.1 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
This solution looks promising. Good job, @gigitux!
As I said above, I didn't check all the cases. For example, my PR doesn't work if a group block doesn't wrap up the Single Product Template. For this reason, it is necessary to increase the unit test coverage and catch all the edge cases.
To solve this, I wonder if instead of wrapping the first block which is not inside a template part, maybe we should:
- Wrap all blocks starting from the first one which is not inside a template part and finishing at the last block before the next template part (aka footer) or the last block in the page in case there is no footer. In other words, wrapping all blocks between the header and the footer instead of only wrapping the first one. That would solve the issue you described, no?
- I would also add a check that inside the blocks we are wrapping, there is the Add to Cart form block or another block we know is required in the Single Product page. That would solve the issue of adding the wrapper when not needed (ie, when using the WooCommerce Classic Template).
Do you think that would work?
| '<!-- wp:group {"className":"woocommerce product"} --> | ||
| <div class="wp-block-group woocommerce product"> | ||
| %1$s | ||
| </div> | ||
| <!-- /wp:group -->', |
There was a problem hiding this comment.
I don't have a strong opinion, but I'm curious to know why you defined a Group block instead of simply a div?
There was a problem hiding this comment.
I'm using parse_blocks function to generate the new structure. In the WordPress template markup, the div is represented with a block group. This is the reason I decided to define a Group block.
|
Thanks for the review!
Yeah, good point! I updated the code :)
👍 |
|
Hey @Aljullu, the PR is ready to review. |
gigitux
added
skip-changelog
…e-investigate-how-to-apply-wc-core-styles-to-product-template
There was a problem hiding this comment.
This looks more robust, but there is still the (edge) case that if somebody adds a block above the Header, this will break:
| Editor | Frontend |
|---|---|
 |
 |
(Notice how the paragraph This is a product and the Breadcrumb block are below the Header in the editor but above it in the frontend)
This is not a straight-forward issue because there are many possible edge cases. What I would try to do:
- Step 1: Split all blocks/template parts. Grouping blocks that don't have template parts in between.
- Step 2: Go through each block/group of blocks and run
has_single_product_template_blocks(). If true, then runcreate_wrap_block_group().
Example
Imagine we have a page with a block above the header and a block below the footer:
- B1 + TP1 + B2 + B3 + B4 + TP2 + B5
(B = Block, TP = Template Part)
Step 1: You would split the blocks/template parts and store them in an array, using TP as delimiters:
- [[B1], [TP1], [B2, B3, B4], [TP2], [B5]]
Step 2: for each item of this array, you would just need to run has_single_product_template_blocks() and create_wrap_block_group() as needed.
If B2, B3 or B4 were a "product template block", you would end up with a wrapper between B2 and B4.
There might be other solutions, but this was the first one that came to my mind. I might have missed some other edge case as well.
Good point. The assumption that I made is wrong. 👍
Having multiple div with the same class could break some extensions, no? 🤔 |
Hmmm, maybe, but some extensions could break as well if some "product template blocks" (ie: Product Gallery and Add to Cart) are not wrapped in |
Yeah, I guess that this is the only way to go. I updated the PR and added another test. |
|
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. |
github-actions
bot
added
the
status: stale
Aljullu
removed
the
status: stale
| } | ||
| $wrapped_blocks = array_map( | ||
| function( $blocks ) use ( $single_product_template_blocks ) { | ||
| $has_single_product_template_blocks = false; |
There was a problem hiding this comment.
Can this line be removed? In L385 we are setting $has_single_product_template_blocks again.
| $last_element_index = count( $carry ) - 1; | ||
| if ( isset( $carry[ $last_element_index ][0]['blockName'] ) && 'core/template-part' !== $carry[ $last_element_index ][0]['blockName'] ) { | ||
| array_push( $carry[ $last_element_index ], $block ); | ||
| return $carry; | ||
| } |
There was a problem hiding this comment.
I guess you can use the end() PHP function here to simplify the code:
$last_element = end( $carry );
if ( isset( $last_element[0]['blockName'] ) && 'core/template-part' !== $last_element[0]['blockName'] ) {
array_push( $last_element, $block );
return $carry;
}There was a problem hiding this comment.
Mmm, I tried with your suggestion but the test failed. I suspect it happens because we don't push the updated element in the carry.
So, I prefer to not change anything here. What do you think?
There was a problem hiding this comment.
It sounds good, I might have missed something. If you keep it as-is, I think you'll need to add an extra check in the case that count( $carry ) is 0, because then the code will try to use $carry[ -1 ] and produce a warning.
| $parsed_blocks = parse_blocks( $template_content ); | ||
| $grouped_blocks = self::group_blocks( $parsed_blocks ); | ||
|
|
||
| $single_product_template_blocks = array( 'woocommerce/breadcrumbs', 'woocommerce/product-gallery-image', 'woocommerce/product-add-to-cart', 'woocommerce/product-details', 'woocommerce/add-to-cart-form' ); |
There was a problem hiding this comment.
When using the classic template, the breadcrumbs are shown outside of the .product div, so I think we should exclude breadcrumbs from this list.
Also woocommerce/product-add-to-cart is used in the Products block, but not in the Single Product template, no? Should we remove it from the list as well? 🤔
gigitux
force-pushed
the
fix/8314-blockifying-single-product-template-investigate-how-to-apply-wc-core-styles-to-product-template
branch
from
f2bc9e7 to
c8cd88e
Compare
…e-investigate-how-to-apply-wc-core-styles-to-product-template
gigitux
deleted the
fix/8314-blockifying-single-product-template-investigate-how-to-apply-wc-core-styles-to-product-template
branch
This is an attempt to fix #8314. This PR is an exploration that I did, and I would like to receive feedback to see if it makes sense to wrap up my work.
Fixes #8314
Screenshots
Testing
Automated Tests
User Facing Testing
Check out this branch.
woocommerceandproduct.WooCommerce Visibility
Performance Impact