-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
19276 - Fixed price renderer issue #19376
19276 - Fixed price renderer issue #19376
Conversation
Hi @sarfarazbheda. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
} else { | ||
$(this.options.slyOldPriceSelector).hide(); | ||
$($product).find(this.options.slyOldPriceSelector).hide(); |
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.
Isn't $product.find
enough? Please put common part into variable or rewrite as
$product.find(...)[condition ? 'show' : 'hide']();
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.
Hello,
I got your point, I will make changes as per your suggestions
} else { | ||
$(this.options.slyOldPriceSelector).hide(); | ||
} | ||
$product.find(this.options.slyOldPriceSelector)[(typeof result != 'undefined' && result.oldPrice.amount !== result.finalPrice.amount) ? 'show' : 'hide'](); |
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.
Teh line is too long now, I would suggest a variable like
isShow = typeof result != 'undefined' && result.oldPrice.amount !== result.finalPrice.amount;
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.
Applied changes and pushed again.
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.
@sarfarazbheda please do not change unneded lines and remove brackets around (isShow)
.
After that squash changes into a single commit.
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.
yes, Made changes and pushed branch again
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.
@sarfarazbheda I still see unneeded lines of code changed. No lines besides 922-926 should be changed, like it was before.
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.
Reverted my old changes and committed code
@sarfarazbheda thanks, changes look good now, please squash them into a single commit, force push and we are good to go 👍 |
As per your comment above, pushed changed into single commit. Let me know once you merge that branch. Thanks. |
Hi @orlangur, thank you for the review. |
@sarfarazbheda thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository. |
✔️ QA passed Was tested by steps described in the initial issue #19276
with
|
Hi @sarfarazbheda, thank you for your contribution! |
Draft release note: When you apply a price rule to a configurable product with swatches, Magento now shows the special price only for the configurable product that has the swatch attribute specified in the price rule. Previously, Magento displayed the special price for all configurable products that had the matching swatch attribute. |
Description (*)
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)