-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fixed #1919 Product price is not changing while changing configurable options #1964
Conversation
If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section. |
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.
Thanks for updating the talon! I'd like to see the changes I requested in the file and also would you please fix the unit tests?
Once you do that this should be good to go.
Also I wasn't able to verify this through my storefront as I don't know which products have a different price for the variants but the path should be correct (and @dpatil-magento will catch anything in QA) :)
const isConfigurable = isProductConfigurable(product); | ||
let productPrice = product.price.regularPrice.amount; | ||
if (isConfigurable) { | ||
const item = findMatchingVariant({ |
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'd like to see this follow the pattern we've used in this file already of using a memo to get some value:
const productPrice = useMemo(
() => getProductPrice(product, optionCodes, optionSelections)
)
The getProductPrice
function could be another top-level utility, it would look similar to getMediaGalleryEntries
:
const getProductPrice = (product, optionCodes, optionSelections) => {
let productPrice = product.price.regularPrice.amount;
if (isProductConfigurable(product)) {
const item = findMatchingVariant({
optionCodes,
optionSelections,
variants
});
productPrice = item.product.price.regularPrice.amount;
}
return productPrice;
}
I was able to get this working with this code:
and then calling it inside
I'm sure there is some optimization that needs to happen there, but it's working right now. |
Nice! that may be the right way to do it instead of my suggestion. The fallback for the |
@@ -214,3 +205,30 @@ export const useProductFullDetail = props => { | |||
quantity | |||
}; | |||
}; | |||
|
|||
const getConfigPrice = (product, optionCodes, optionSelections) => { |
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.
For consistency please put this function up next to the other helper functions in this file.
@@ -200,3 +205,30 @@ export const useProductFullDetail = props => { | |||
quantity | |||
}; | |||
}; | |||
|
|||
const getConfigPrice = (product, optionCodes, optionSelections) => { | |||
let value = []; |
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.
This can just be let value;
as it will get set later. Also an array as a default seems wrong anyways.
Fixed #1919 Product price is not changing while changing configurable options
To reproduce
Steps to reproduce the behavior:
Expected behavior
The product price should change with the options
Related Issue
Closes #1919