fix(storefront): STRF-4701 - Fixed a use case that prevented retail/sale prices from displaying on PDP #1262

bc-jz · 2018-06-08T16:36:21Z

What?

Updated PDP price and price-range templates to always include containing elements for rrp, non-sale, and sale prices even if they are not present in the context on initial page load. This ensures the proper elements are available to be updated by Javascript when option seleections are made that do have any of the prices available.

Tickets / Documentation

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

Screenshots (if appropriate)

Pricing behavior examples:
https://docs.google.com/document/d/1rXL4t6fQh70bHZ6mnoPvvFcVIh9ZOjcLtfAdlPSqFEc/edit?usp=sharing

bigbot · 2018-06-08T16:36:22Z

Autotagging @bigcommerce/storefront-team @davidchin

bc-jz · 2018-06-08T16:39:09Z

templates/components/products/price-range.html

+        <div class="price-section price-section--withoutTax rrp-price--withoutTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
+            {{lang 'products.retail_price'}}
+            <span data-product-rrp-price-without-tax class="price price--rrp">{{price.retail_price_range.min.without_tax.formatted}} - {{price.retail_price_range.max.without_tax.formatted}}</span>
+            {{#if schema_org}}


I am curious about this meta content......I included it because it was already in the code that I was sampling but does it make sense for us to have these meta pricing properties for a retail price range?

I noticed that regarding an individual variant price, we do not include meta properties for a retail price - only the actual price. So maybe these metas are safe to leave off of retail pricing or they should be added to the retail price for non-range pricing.

ziad-abdo

Good idea with the conditionals around the styling. As long as the js properly toggles back and forth, I think this will work.

ziad-abdo · 2018-06-08T16:38:48Z

templates/components/products/price-range.html

+            <div class="price-section price-section--withTax rrp-price--withTax" style="display: none;">
+            {{lang 'products.retail_price'}}
+            <span data-product-rrp-with-tax class="price price--rrp">
+                {{#if price.rrp_with_tax}}


might want to have this if check rest above {{lang 'products.retail_price'}} that way it doesnt show the retail price text without there being a price to display.

The retail price text will not show until JS updates this element to no longer be hidden.

ziad-abdo · 2018-06-08T16:44:05Z

templates/components/products/price.html

+        <div class="price-section price-section--withTax rrp-price--withTax" {{#unless price.rrp_with_tax}}style="display: none;"{{/unless}}>
+            {{lang 'products.retail_price'}}
+            <span data-product-rrp-with-tax class="price price--rrp">
+                {{#if price.rrp_with_tax}}


same here with moving the if statement up a bit? maybe you had a reason for showing the text though.

Ah, actually, if I understand this right, you can probably take these if statements out entirely-- since you're basically changing the way you chose wether or not to display with the style="display: none;" That should probably do it.

I think I need the IF statement because I am not sure if price.rrp_with_tax exists....not sure how stencil acts if I try to print a value that is not an actual property. The placement of the IF cannot be moved up though because I need to always print the SPAN whether or not it has a value.

The style display none ensures that the "Retail price" label will not be seen unless there is a value to print or until toggled on by the JS.

I just mean that you're already checking if theres a value for price with tax above:

<div class="price-section price-section--withTax rrp-price--withTax" {{#unless price.rrp_with_tax}}style="display: none;"{{/unless}}>

you dont probably need it again

{{#if price.rrp_with_tax}}

right?

I'll test what happens if I exclude it....the {{#unless..}} conditional will make the element hide when there is not a retail price value to enter....im just not sure how stencil will react if i try to access a non-existent property

hmmmm, so looks like you are right:

Using:

{{#if price.rrp_without_tax}} {{price.rrp_without_tax.formatted}} {{/if}}

is functionally the same as just calling {{price.rrp_without_tax.formatted}}

If the property exists then it prints it, if not it prints nothing....which is what I want. I was just following the existing code conventions I saw so I assumed it would be a problem to try to print a property that did not exist...

I guess previously the IF was wrapping around HTML as well as the property so that was just a bad assumption on my part as to why the IF statement was there.

…tes to always include containing elements for rrp, non-sale, and sale prices even if they are not present in the context on initial page load. This ensures the proper elements are available to be updated by Javascript when option seleections are made that do have any of the prices available.

… requested in PR and made the PDP pricing labels editable in the Theme customization settings.

…ta tags that were tied to Retail Price Range. It looks like schema.org does not support pricing types other than the actual product price at this time.

ziad-abdo

couple questions and concerns here and there but overall looking good. Is there a chance with this implementation to show rrp/price twice? Its a little tough keeping up with the conditionals, but if it covers the cases we know about its looking good to me.

ziad-abdo · 2018-06-11T17:44:28Z

config.json

-    "price_ranges": true
+    "price_ranges": true,
+    "pdp-price-label": "",
+    "pdp-sale-price-label": "Now:",


is there a benefit of using these over lang? should the lang strings be removed now?

The benefit is easier access to changing these values by a client. It allows them to set the value they want via the "Theme Customization" setting. It looks like this:

very nice, dont want to remove the old strings from whatever lang files where in it before?

Thanks for the reminder! I will do that. I had meant to run a search on those language values and make sure they are not used elsewhere, then remove them if not.

ziad-abdo · 2018-06-11T18:09:23Z

templates/components/products/price-range.html

+        {{theme_settings.pdp-retail-price-label}}
+        <span data-product-rrp-price-with-tax class="price price--rrp">{{price.retail_price_range.min.with_tax.formatted}} - {{price.retail_price_range.max.with_tax.formatted}}</span>
+    </div>
+    {{else}}


does this else block need to close somewhere? not sure which if its connected to.

This else block is connected to the {{#and}} on line 1 - it is the alternate path when the {{#and}} fails. It does not need it's own closing line as it closes with the {{/and}}

ah, then it should share indentation with the and on line 1

that makes sense! It threw me off because it does not have it's own ending line, but you are right the indentation would be more clear that way

ziad-abdo · 2018-06-11T18:12:50Z

templates/components/products/price-range.html

 {{/and}}
 {{#and price.price_range.min.with_tax price.price_range.max.with_tax}}
+    <div class="price-section price-section--withTax non-sale-price--withTax" style="display: none;">


Interesting that you chose to default with display: none here, should this also have an unless block around it?

At least for right now a sale price should never display if a price range is being displayed. At least that is my thinking. I want the element to be on the page but want to avoid having a sale_price from the lowest priced variant display next to a price range.

ziad-abdo · 2018-06-11T18:14:01Z

templates/components/products/price-range.html

+    <div class="price-section price-section--withTax non-sale-price--withTax" style="display: none;">
+        {{theme_settings.pdp-non-sale-price-label}}
+        <span data-product-non-sale-price-with-tax class="price price--non-sale">
+            {{price.non_sale_price_with_tax.formatted}}


Price range, I think, is associated with calculated_price which I think would be like "sale price" in this context? worth double checking, but I'm assuming if you dont have a range then you want to display the more closely associated price value.

The "price_range" we generate will take any sale price that is affecting a variant into account. As a result the "price range" which we are displaying in the price-section--withTax block covers both sale and non-sale prices. The price range does not have a concept of letting the user know whether or not the prices included in it's range are on sale.

This "sale price" HTML block we have here though is not intended to display a price range. It is specifically to for use for displaying the "non-sale price" of a variant after making option selections on the page.

I must be confused about something, it seems like if options are selected, then what we want to be showing is price.without_tax or price.with_tax as the calculated price for the selection-- and non_sale_price is a display price to reflect savings. I'm probably missing something here.

Once options are selected we do want to show the price.without_tax or price.with_tax. We also want to show the price.non_sale_price_without_tax or price.non_sale_price_with_tax. The HTML you are referencing with this comment facilitates the latter.

ziad-abdo · 2018-06-11T18:16:01Z

templates/components/products/price-range.html

    <div class="price-section price-section--withTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
+        <span class="price-label">{{theme_settings.pdp-price-label}}</span>
+        <span class="price-now-label" style="display: none;">{{theme_settings.pdp-sale-price-label}}</span>


same concern here with the unless block. when the page loads and there is a price range, do you see it at first not display and then suddenly display after the js loads?

You will see the page load with a price range which then changes to an individual price if you have default option selections enabled on the product and it has a price range. This is the same issue that exists right now, this PR unfortunately does not address that issue. I don't think that I can correct this JS loading issue by just adding an "unless" statement here. I'd either need to add some logic that tries to identify if there are any default options selected (which can be parsed from the option value content in the page context) or I would need to add a new property to the context that gives me this information (a has_default_options boolean property). I am aiming for the latter but it did not fit into the scope of this PR.

If you have a suggestion on how to use the "unless" though that maybe I am missing then lets discuss.

bc-jz · 2018-06-11T19:07:38Z

templates/components/products/price-range.html

@@ -33,8 +40,31 @@
        {{/if}}
    </div>
 {{/and}}
+{{#and price.retail_price_range.min.without_tax price.retail_price_range.max.without_tax}}


I moved the without_tax range (and other without tax elements) below the with tax elements to make sure the order of these elements does not differ between the use case of having a price range on initial page load vs having a single price. On the single price display (seen in prices.html) we also have all of the with tax elements above the without_tax elements in a descending order of retail price, non-sale price, and sale price (or price). I believe the way I have this page ordered will ensure that both codepaths produce the same order of displaying prices.

ziad-abdo · 2018-06-11T18:18:57Z

templates/components/products/price.html

@@ -31,30 +32,31 @@
                </div>
            {{/if}}
            {{#if price.without_tax}}
-                <abbr title="{{lang 'products.excluding_tax'}}">{{lang 'products.price_with_tax' tax_label=price.tax_label}}</abbr>
+                <abbr title="{{lang 'products.including_tax'}}">{{lang 'products.price_with_tax' tax_label=price.tax_label}}</abbr>


🙃 im not sure whats going on here: the if check block is {{#if price.without_tax}} but then we're showing including tax?

Yep, this is specifically for the use case of displaying both including and excluding tax prices on the page. When the condition passes we have both price including and excluding tax in our context. In such a case we want to display identifying text after the price (including tax) and (excluding tax), respectively.

Using this specific line as an example, we have immediately before this point printed the price including tax. So if the context also has a price excluding tax, then we need to print (Including tax) to the page to sit right after the including tax price we printed a few lines above.

…in PR review: indention correction and removing language file properties that are no longer being used in the template.

ziad-abdo

good work 👍

CHANGELOG entry for PR #1262

bc-jz commented Jun 8, 2018

View reviewed changes

ziad-abdo reviewed Jun 8, 2018

View reviewed changes

bc-jz added 3 commits June 11, 2018 11:24

fix(storefront): STRF-4701 - Removed from extraneous IF statements as…

8dd15c2

… requested in PR and made the PDP pricing labels editable in the Theme customization settings.

fix(storefront): STRF-4701 - Removed schema.org priceSpecification me…

58f8388

…ta tags that were tied to Retail Price Range. It looks like schema.org does not support pricing types other than the actual product price at this time.

bc-jz force-pushed the STRF-4701 branch from fc35e71 to 58f8388 Compare June 11, 2018 16:29

ziad-abdo reviewed Jun 11, 2018

View reviewed changes

fix(storefront): STRF-4701 - Some final cleanup that was pointed out …

e839bbb

…in PR review: indention correction and removing language file properties that are no longer being used in the template.

ziad-abdo approved these changes Jun 11, 2018

View reviewed changes

bc-jz merged commit c61dc67 into bigcommerce:master Jun 12, 2018

mike-wilson-css mentioned this pull request Jun 12, 2018

CHANGELOG entry for PR #1262 #1268

Merged

mattolson added a commit that referenced this pull request Jun 12, 2018

Merge pull request #1268 from sacr3dc0w/changelog-1262

140103a

CHANGELOG entry for PR #1262

bc-jz mentioned this pull request Jun 20, 2018

STRF-4701 - Additional fixes to address missing class names #1277

Merged

flair-duncan mentioned this pull request Jun 30, 2018

AMP Test no longer validates since PR#1262 #1297

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(storefront): STRF-4701 - Fixed a use case that prevented retail/sale prices from displaying on PDP #1262

fix(storefront): STRF-4701 - Fixed a use case that prevented retail/sale prices from displaying on PDP #1262

bc-jz commented Jun 8, 2018

bigbot commented Jun 8, 2018

bc-jz Jun 8, 2018

ziad-abdo left a comment

ziad-abdo Jun 8, 2018

bc-jz Jun 8, 2018

ziad-abdo Jun 8, 2018

ziad-abdo Jun 8, 2018

bc-jz Jun 8, 2018 •

edited

Loading

ziad-abdo Jun 8, 2018

bc-jz Jun 8, 2018

bc-jz Jun 8, 2018 •

edited

Loading

ziad-abdo left a comment

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018 •

edited

Loading

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

bc-jz Jun 11, 2018

This comment was marked as resolved.

bc-jz Jun 11, 2018

ziad-abdo Jun 11, 2018

bc-jz Jun 11, 2018

ziad-abdo left a comment

fix(storefront): STRF-4701 - Fixed a use case that prevented retail/sale prices from displaying on PDP #1262

fix(storefront): STRF-4701 - Fixed a use case that prevented retail/sale prices from displaying on PDP #1262

Conversation

bc-jz commented Jun 8, 2018

What?

Tickets / Documentation

Screenshots (if appropriate)

bigbot commented Jun 8, 2018

Choose a reason for hiding this comment

ziad-abdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc-jz Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc-jz Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

ziad-abdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziad-abdo Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziad-abdo left a comment

Choose a reason for hiding this comment

bc-jz Jun 8, 2018 •

edited

Loading

bc-jz Jun 8, 2018 •

edited

Loading

ziad-abdo Jun 11, 2018 •

edited

Loading