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

Catalog 2406 #1195

Merged
merged 1 commit into from
Apr 9, 2018
Merged

Catalog 2406 #1195

merged 1 commit into from
Apr 9, 2018

Conversation

bc-vandana
Copy link
Contributor

@bc-vandana bc-vandana commented Mar 26, 2018

What?

Adding stencil logic for the rrp strikethrough on PDP and PLP

Tickets / Documentation

https://jira.bigcommerce.com/browse/CATALOG-2406 has the mockups for all the scenarios for products and variants.

Screenshots (if appropriate)

https://jira.bigcommerce.com/browse/CATALOG-2406

Tests

STORE_DOMAIN="https://store-hkyr4dowa1.my-integration.zone" STORE_EMAIL="vandana.premkumar+1522082908@bigcommerce.com" STORE_PASSWORD="db421f1fddd534d3800a" ./functional_phpunit.sh --filter PriceListsStoreFrontPLPUITest --group V3

Starting Functional Tests against specified store:
Cleaning up screenshots...

Domain: https://store-hkyr4dowa1.my-integration.zone
Email: vandana.premkumar+1522082908@bigcommerce.com
Password: db421f1fddd534d3800a

PHP Warning: Module 'pcntl' already loaded in Unknown on line 0

Warning: Module 'pcntl' already loaded in Unknown on line 0
STORE Hash URL: https://store-hkyr4dowa1.my-integration.zone

Build commit id: fea802701841281c261c4c1edcc7547454d0b455
PHPUnit 6.0.8 by Sebastian Bergmann and contributors.

Runtime: PHP 7.0.27 with Xdebug 2.6.0
Configuration: /Users/vandana.premkumar/app/bigcommerce-app-vm/codebases/bigcommerce/tests/Functional/functional.xml

.... 4 / 4 (100%)

Time: 2.69 minutes, Memory: 76.00MB

OK (4 tests, 89 assertions)

STORE_DOMAIN="https://store-hkyr4dowa1.my-integration.zone" STORE_EMAIL="vandana.premkumar+1522082908@bigcommerce.com" STORE_PASSWORD="db421f1fddd534d3800a" ./functional_phpunit.sh --filter PriceListsStoreFrontPDPUITest --group V3
selenium-server-standalone-3.5.0.jar already exists, skipping download
Gecko driver exists, skipping download
Starting Selenium Server with options '-Dwebdriver.gecko.driver=./geckodriver_v0.17.0 -jar ./selenium-server-standalone-3.5.0.jar -enablePassThrough false' in the background
Selenium Server started with PID 87827
To reuse this store for future requests, run:
STORE_DOMAIN="https://store-hkyr4dowa1.my-integration.zone" STORE_EMAIL="vandana.premkumar+1522082908@bigcommerce.com" STORE_PASSWORD="db421f1fddd534d3800a" ./functional_phpunit.sh --filter PriceListsStoreFrontPDPUITest --group V3

Starting Functional Tests against specified store:
Cleaning up screenshots...

Domain: https://store-hkyr4dowa1.my-integration.zone
Email: vandana.premkumar+1522082908@bigcommerce.com
Password: db421f1fddd534d3800a

PHP Warning: Module 'pcntl' already loaded in Unknown on line 0

Warning: Module 'pcntl' already loaded in Unknown on line 0
STORE Hash URL: https://store-hkyr4dowa1.my-integration.zone

Build commit id: fea802701841281c261c4c1edcc7547454d0b455
PHPUnit 6.0.8 by Sebastian Bergmann and contributors.

Runtime: PHP 7.0.27 with Xdebug 2.6.0
Configuration: /Users/vandana.premkumar/app/bigcommerce-app-vm/codebases/bigcommerce/tests/Functional/functional.xml

.......... ..... 15 / 15 (100%)

Time: 14.81 minutes, Memory: 78.00MB

OK (15 tests, 268 assertions)
Functional Tests complete

@bookernath @junedkazi @mjschock

lang/en.json Outdated
@@ -674,7 +674,9 @@
"choose_an_option": "Please choose an option",
"select_one": "Please select one",
"description": "Description",
"retail_price": "MSRP",
"retail_price": "MSRP:",
"was": "Was:",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we prefix value with price like price_was. Same for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

lang/en.json Outdated
@@ -687,7 +689,8 @@
"metric": "cm",
"imperial": "in"
},
"you_save": "(You save {amount})",
"you_save_1": "(You save",
"you_save_2": ")",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should change it to a single variable you_save like (You save {price}). And then you can call it as {{lang 'products.you_save' price=price.saved.formatted}}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That unfortunately does not work, and is causing the bug CATALOG-3121. Which is the main reason for the split

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make that change & show it to me or Michael. I am curious why it does not work.

Copy link
Contributor Author

@bc-vandana bc-vandana Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see the first "you save" has the accurate value, but the second "you save" one that uses {{lang 'products.you_save' price=price.saved.formatted}} does not update the value.
screen shot 2018-03-26 at 4 43 47 pm
screen shot 2018-03-26 at 4 44 10 pm

</span>
<span class="price">{{lang 'products.you_save_2'}}</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this based on the suggestions above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That unfortunately does not work, and is causing the bug CATALOG-3121. Which is the main reason for the split

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this seems fine because we are dealing with updating the already-rendered view here and so can't pass it to the helper at this point. but let's name these something more descriptive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm wondering - maybe we could simply have something like a you_save = "You save" and then include the parens as part of the html or css so we could have:

<span class="price">{{lang 'products.you_save'}}
<span data-product-price-saved class="price">
  {{price.saved.formatted}}
</span>

::before {
content: "(";
}

::after {
content: ")";
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easiest would be in html just in case css messes up. You would always be 100% sure it works with html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

splitting it in en.json seems the best for now. But we will need to fix the underlining problem of having a dynamic value to be added as a parameter.

Copy link
Contributor

@mjschock mjschock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add an entry to the changelog and squash the commits into one

"retail_price": "MSRP",
"retail_price": "MSRP:",
"price_was": "Was:",
"price_now": "Now:",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍹 in my opinion we would leave the : out of the lang file and in the template directly, @junedkazi do you have an opinion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you could argue the colon is part of the language. we also have it in MSRP: above so i think think it's fine to stay

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

@bigbot
Copy link

bigbot commented Mar 27, 2018

Autotagging @bigcommerce/storefront-team @davidchin

@bc-vandana
Copy link
Contributor Author

I added more code yesterday to fix some UI fixes where ajax response values are not showing up in UI when variant is modified. JZ is testing in an integration store that has the updated theme.
http://vandanapremkumar1522343934-testington.my-integration.zone/

@bc-jz
Copy link
Contributor

bc-jz commented Apr 2, 2018

This PR is looking good. The only lingering issue I am seeing is that if retail or sale price are not showing at the time of page load then they will not show when choosing variants that have retail and/or sale price values. This is a problem with our current template as well though and I have created a JIRA to document:

https://jira.bigcommerce.com/browse/STRF-4701

💚

@bc-vandana bc-vandana force-pushed the CATALOG-2406 branch 2 times, most recently from ad180dd to 13f01be Compare April 9, 2018 16:57
…e and non sale price. Also, had to add a hack to fix an existing bug where the price.saved value is not getting updated based on change in variant when price_range flag is turned off. Add changelog for the ticket. Adding the price_ prefix for was and now to implement PR comments.
@bc-vandana bc-vandana merged commit 3de6916 into bigcommerce:master Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants