This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Product Gallery Block > Remove global variable overwrite and keep support for the Single Product Block. #9458
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
…ock to continue to work within the Single Product template without any problems.
nefeline
changed the base branch from
trunk
to
fix/remove-call-to-global-variables-from-blocks
|
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. |
4 tasks
|
Size Change: 0 B Total Size: 1.08 MB ℹ️ View Unchanged
|
…le within the ProductImageGallery class for rendering the Product Image block.
nerrad
reviewed
May 15, 2023
Comment on lines
+42
to
+44
| ob_start(); | ||
| echo sprintf( '<span class="onsale">%1$s</span>', esc_html__( 'Sale!', 'woo-gutenberg-products-block' ) ); | ||
| return ob_get_clean(); |
There was a problem hiding this comment.
No need to use an output buffer here since you have control over the output. Just return it.
There was a problem hiding this comment.
If this is a copy of the classic template, it'd be better to pull in the template directly via Woo template functions. That way if the theme has any template override it'll use the override. It's also possible (but rare) that a plugin (or custom code) might be adding a template override.
I'd also want to avoid just copying template code into here (along with the filters) because it gives the impression this will be the long-term version of the block and we're committing to these filters long-term in this context. I'd prefer to leave room for potential iterations that might not/likely won't expose these filters.
nerrad
reviewed
May 15, 2023
| return ''; | ||
| } | ||
|
|
||
| ob_start(); |
There was a problem hiding this comment.
Output buffer isn't needed here, just save the output from the loop to a variable and then return it.
nerrad
reviewed
May 15, 2023
Comment on lines
+145
to
+148
| $single_post = get_post( $post_id ); | ||
| if ( ! $single_post instanceof \WP_Post ) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
Posts with product post_type will be an instance of WP_Post, why return? These conditionals seem unnecessary.
nerrad
reviewed
May 15, 2023
| return ''; | ||
| } | ||
|
|
||
| $single_product = wc_get_product( $post_id ); |
There was a problem hiding this comment.
The single prefix on the variable name is redundant. You can still use product.
nerrad
reviewed
May 15, 2023
| * | ||
| * @return string | ||
| */ | ||
| private function product_image( $product, $post ) { |
There was a problem hiding this comment.
$post isn't used by the function.
nerrad
reviewed
May 15, 2023
| * @param \WC_Product $product Product instance. | ||
| * @param \WP_Post $post Post instance. | ||
| */ | ||
| private function sale_flash( $product, $post ) { |
There was a problem hiding this comment.
$post isn't used by the function.
nerrad
reviewed
May 15, 2023
There was a problem hiding this comment.
I realize this PR is still draft. I'd suggest just focus on fixing the global in this PR and the other changes can be made in a separate PR. Those changes are questionable in their current state and keeping in a separate PR can allow for the more pressing issue of the global overwrite to be dealt with sooner.
Sounds good! I'm cherry-picking the commit for the global variable change and opening a separate PR for that.
Yep! They are 100% questionable :D I did this in 20 minutes by quickly adapting the code of those 3 templates and consolidating them in our class as a proof of concept for the discussion kicked off on #9457: I didn't want to go into details here without gathering folks opinions first, in case someone had any serious concerns with us following this path. I'll be working on this and another separate PR for the Add to Cart with Options blocks so we can discuss what should be effectively ported over from core and what should be ditched. |
…ix/remove-call-to-global-variables-from-gallery-block
4 tasks
Base automatically changed from
fix/remove-call-to-global-variables-from-blocks
to
trunk
May 16, 2023 08:18
|
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
github-actions
bot
removed
the
status: stale
|
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
status: stale
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.
Ensure the
$productglobal variable is not overwritten within theProduct Galleryblock while still keeping support for the (still experimental) Single Product block.Additionally, the following templates are also being incorporated to the
ProductImageGalleryclass:single-product/product-thumbnails.phpsingle-product/product-image.phpsingle-product/sale-flash.phpThose are added as a viable alternative for allowing the render of the Product Gallery Block as an inner block for the Single Product Block. In other words, this change enables the usage of the Product Gallery Block outside of the scope of the Single Product template by not relying on the global
$productvariable, but on the context of the Single Product block instead.Fixes #9453
Testing
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog