Skip to content
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

Merged
merged 1 commit into from
Mar 11, 2019
Merged

19276 - Fixed price renderer issue #19376

merged 1 commit into from
Mar 11, 2019

Conversation

sarfarazbheda
Copy link
Contributor

@sarfarazbheda sarfarazbheda commented Nov 24, 2018

Description (*)

Fixed Issues (if relevant)

  1. Change different product price on selecting different product swatches on category pages #19276: Fixed price renderer issue

Manual testing scenarios (*)

  1. Select Any category, select Swatches (color and size) from category page. Price will reflect on the current product after selecting color and size.(i.e. Swatch)

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

Sorry, something went wrong.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 24, 2018

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @sarfarazbheda. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

} else {
$(this.options.slyOldPriceSelector).hide();
$($product).find(this.options.slyOldPriceSelector).hide();
Copy link
Contributor

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']();

Copy link
Contributor Author

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']();
Copy link
Contributor

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;

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

@orlangur
Copy link
Contributor

@sarfarazbheda thanks, changes look good now, please squash them into a single commit, force push and we are good to go 👍

@sarfarazbheda
Copy link
Contributor Author

As per your comment above, pushed changed into single commit. Let me know once you merge that branch. Thanks.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.1 milestone Dec 3, 2018
@magento-engcom-team magento-engcom-team added Partner: Krish TechnoLabs partners-contribution Pull Request is created by Magento Partner labels Dec 3, 2018
@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-3608 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@sarfarazbheda thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@sdzhepa
Copy link
Contributor

sdzhepa commented Mar 5, 2019

✔️ QA passed

Was tested by steps described in the initial issue #19276

  1. Created swatches
  2. Created several Configurable products with the same swatch color
  3. Created Catalog Price Rule to apply a discount (special price)
  4. Applied changes to the final_price.phtml template file as mentioned in https://magento.stackexchange.com/questions/196090/magento-2-display-old-price-and-special-price-in-category-list
    Replace
<?php if (!$block->isProductList() && $block->hasSpecialPrice()): ?>
<span class="old-price sly-old-price no-display">

with

<?php if ($block->hasSpecialPrice()): ?>
<span class="old-price sly-old-price">
  1. Flush caches and reindex
  2. Go to Storefront on Catgory page

Result
pr19376

@ghost
Copy link

ghost commented Mar 11, 2019

Hi @sarfarazbheda, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.2 milestone Mar 11, 2019
magento-engcom-team pushed a commit that referenced this pull request Mar 11, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@meker12
Copy link
Contributor

meker12 commented Jun 3, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants