This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Product Price block: add support to the Single Product Template #8609
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
woocommercebot
requested review from
a team and
imanish003
and removed request for
a team
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/atomic/blocks/product-elements/price/edit.tsx
assets/js/atomic/blocks/product-elements/price/index.tsx assets/js/atomic/blocks/product-elements/shared/product-selector.tsx |
gigitux
force-pushed
the
8479-product-price-block-add-support-to-the-single-product-template
branch
3 times, most recently
from
dc25b45 to
3d8a090
Compare
|
Size Change: +392 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
gigitux
force-pushed
the
8479-product-price-block-add-support-to-the-single-product-template
branch
from
3d8a090 to
5205f52
Compare
gigitux
added
type: enhancement
gigitux
changed the title
thealexandrelara
approved these changes
Mar 2, 2023
There was a problem hiding this comment.
Great work! I added a comment in the code to check your opinion about it, but I'm pre-approving it.
One thing that I noticed is that the font-size for the Product Price seems smaller than the original one (in the Classic template), just want to confirm if this is expected or not:
| Classic | Current |
|---|---|
 |
 |
| regularPrice={ | ||
| isDescendentOfSingleProductTemplate | ||
| ? pricePreview | ||
| : prices.price |
There was a problem hiding this comment.
I'm wondering if since the lines 94-96 are using the same logic as these lines, if it would be better to store the logic in a variable/function. Just a precaution to prevent from someone from changing only one of them and forgot to change the other piece of the code in the future. Maybe something like this would work:
const price = isDescendentOfSingleProductTemplate
? pricePreview
: prices.price
...
return (
<ProductPrice
...
price={price}
...
regularPrice={price}
/>
)
What do you think?
There was a problem hiding this comment.
Good point, but for the regular price props, it is necessary to use .regular_price. I updated the code!
imanish003
suggested changes
Mar 3, 2023
There was a problem hiding this comment.
Hi @gigitux, Well done! Except for minor things, everything else looks great to me 🙌🏻
I have left some minor suggestions but I don't have strong opinion about all of those 🙂
…ocks into 8479-product-price-block-add-support-to-the-single-product-template
|
Thanks folks, for your great comments! @imanish003 I addressed all your feedback! 💪
Yep, the goal is to use the Global Styles to change the dimension of the text. By default, we will set Global Styles in the default HTML template. So, it will be necessary to update #8515 |
thealexandrelara
approved these changes
Mar 3, 2023
There was a problem hiding this comment.
Nice! Thank you for addressing all the raised points! LGTM! 🚀
Yep, the goal is to use the Global Styles to change the dimension of the text. By default, we will set Global Styles in the default HTML template. So, it will be necessary to update #8515
Cool, so after we merge this one I'll add it to the Default HTML Template
…-single-product-template
This was referenced Mar 3, 2023
Merged
…-single-product-template
imanish003
approved these changes
Mar 6, 2023
| onClick={ () => setIsEditing( true ) } | ||
| > | ||
| { __( | ||
| 'Switch product…', |
There was a problem hiding this comment.
May I know if there is any reason behind adding ... for Switch product…' text?
There was a problem hiding this comment.
I don't know. It is the same string that there is in WithProductSelector HOC https://github.com/woocommerce/woocommerce-blocks/blob/e4325c12dd3bca8bb236454e9af90ce6416f6940/assets/js/atomic/blocks/product-elements/shared/with-product-selector.js/#L79-L80
…-single-product-template
gigitux
deleted the
8479-product-price-block-add-support-to-the-single-product-template
branch
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.
This PR added support to the Product Price block to Single Product Template.
Fixes #8479
On the implementation level, the main issue is that the atomic blocks support several contexts:
All Products Block->withProductDataContextHOCProducts Block-> Gutenberg ContextSingle Product Template-> Gutenberg ContextWithSelectorandwithProductDataContextHOCswithProductDataContext
The HOC withProductDataContext servers pass the select product object to the block.
With Selector
The HOC withSelector, when the
ProductDataContextisn't defined, renders a modal for selecting the product and setting the product id in the context. In this way, the block can fetch the right data.We added the support to different contexts at different times, so the code isn't in the best shape, which influenced the work for this PR.
It was necessary to create a component identical to the ´WithSelector` HOC. There is no need to show the selector product modal when the block is inside the Single Product template. Using a component, we can easily distinguish the two cases.
In any case, we should refactor all the atomic blocks and create a common logic easily to check and with great type safety: should we put it as a cooldown task?
Testing
User Facing Testing
WooCommerce Visibility
Changelog