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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Jun 12, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5de8070
fix(storefront): STRF-4701 - Updated PDP price and price-range templa…
bc-jz Jun 8, 2018
8dd15c2
fix(storefront): STRF-4701 - Removed from extraneous IF statements as…
bc-jz Jun 11, 2018
58f8388
fix(storefront): STRF-4701 - Removed schema.org priceSpecification me…
bc-jz Jun 11, 2018
e839bbb
fix(storefront): STRF-4701 - Some final cleanup that was pointed out …
bc-jz Jun 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions assets/js/theme/common/product-details.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,12 @@ export default class ProductDetails {
$div: $('.rrp-price--withoutTax', $scope),
$span: $('[data-product-rrp-price-without-tax]', $scope),
},
nonSaleWithPrice: {
$div: $('.non-sale-price---withTax', $scope),
nonSaleWithTax: {
$div: $('.non-sale-price--withTax', $scope),
$span: $('[data-product-non-sale-price-with-tax]', $scope),
},
nonSaleWithoutPrice: {
$div: $('.non-sale-price---withoutTax', $scope),
nonSaleWithoutTax: {
$div: $('.non-sale-price--withoutTax', $scope),
$span: $('[data-product-non-sale-price-without-tax]', $scope),
},
priceSaved: {
Expand All @@ -109,6 +109,9 @@ export default class ProductDetails {
priceNowLabel: {
$span: $('.price-now-label', $scope),
},
priceLabel: {
$span: $('.price-label', $scope),
},
$weight: $('.productView-info [data-product-weight]', $scope),
$increments: $('.form-field--increments :input', $scope),
$addToCart: $('#form-action-addToCart', $scope),
Expand Down Expand Up @@ -378,10 +381,11 @@ export default class ProductDetails {
clearPricingNotFound(viewModel) {
viewModel.rrpWithTax.$div.hide();
viewModel.rrpWithoutTax.$div.hide();
viewModel.nonSaleWithPrice.$div.hide();
viewModel.nonSaleWithoutPrice.$div.hide();
viewModel.nonSaleWithTax.$div.hide();
viewModel.nonSaleWithoutTax.$div.hide();
viewModel.priceSaved.$div.hide();
viewModel.priceNowLabel.$span.hide();
viewModel.priceLabel.$span.hide();
}

/**
Expand All @@ -392,10 +396,12 @@ export default class ProductDetails {
this.clearPricingNotFound(viewModel);

if (price.with_tax) {
viewModel.priceLabel.$span.show();
viewModel.$priceWithTax.html(price.with_tax.formatted);
}

if (price.without_tax) {
viewModel.priceLabel.$span.show();
viewModel.$priceWithoutTax.html(price.without_tax.formatted);
}

Expand All @@ -415,15 +421,17 @@ export default class ProductDetails {
}

if (price.non_sale_price_with_tax) {
viewModel.nonSaleWithPrice.$div.show();
viewModel.priceLabel.$span.hide();
viewModel.nonSaleWithTax.$div.show();
viewModel.priceNowLabel.$span.show();
viewModel.nonSaleWithPrice.$span.html(price.non_sale_price_with_tax.formatted);
viewModel.nonSaleWithTax.$span.html(price.non_sale_price_with_tax.formatted);
}

if (price.non_sale_price_without_tax) {
viewModel.nonSaleWithoutPrice.$div.show();
viewModel.priceLabel.$span.hide();
viewModel.nonSaleWithoutTax.$div.show();
viewModel.priceNowLabel.$span.show();
viewModel.nonSaleWithoutPrice.$span.html(price.non_sale_price_without_tax.formatted);
viewModel.nonSaleWithoutTax.$span.html(price.non_sale_price_without_tax.formatted);
}
}

Expand Down
1 change: 1 addition & 0 deletions assets/scss/components/stencil/price/_price.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// =============================================================================

.price--rrp,
.price--non-sale,
.price--discounted {
text-decoration: line-through;
}
Expand Down
6 changes: 5 additions & 1 deletion config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,11 @@
"geotrust_ssl_common_name": "",
"geotrust_ssl_seal_size": "M",
"navigation_design": "simple",
"price_ranges": true
"price_ranges": true,
"pdp-price-label": "",
"pdp-sale-price-label": "Now:",
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a benefit of using these over lang? should the lang strings be removed 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.

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:

2018-06-11_1144

Copy link
Contributor

Choose a reason for hiding this comment

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

very nice, dont want to remove the old strings from whatever lang files where in it 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.

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.

"pdp-non-sale-price-label": "Was:",
"pdp-retail-price-label": "MSRP:"
},
"read_only_files": [
"/assets/scss/components/citadel",
Expand Down
3 changes: 0 additions & 3 deletions lang/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,9 +674,6 @@
"choose_an_option": "Please choose an option",
"select_one": "Please select one",
"description": "Description",
"retail_price": "MSRP:",
"price_was": "Was:",
"price_now": "Now:",
"price_with_tax": "(Inc. {tax_label})",
"price_without_tax": "(Ex. {tax_label})",
"including_tax": "Including Tax",
Expand Down
24 changes: 24 additions & 0 deletions schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,6 +1117,30 @@
"type": "heading",
"content": "Product pages"
},
{
"type": "text",
"label": "Product sale price label",
"id": "pdp-sale-price-label"
},
{
"type": "text",
"label": "Product before sale price label",
"id": "pdp-non-sale-price-label"
},
{
"type": "text",
"label": "Product retail price label",
"id": "pdp-retail-price-label"
},
{
"type": "text",
"label": "Product price label",
"id": "pdp-price-label"
},
{
"type": "paragraph",
"content": "* the 'Product price label' is displayed when there is not a sale price."
},
{
"label": "Product swatch image sizes",
"type": "imageDimension",
Expand Down
1 change: 1 addition & 0 deletions templates/components/amp/css/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@
}

.price--rrp,
.price--non-sale,
.price--discounted {
text-decoration: line-through;
}
58 changes: 44 additions & 14 deletions templates/components/products/price-range.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,28 @@
{{#and price.retail_price_range.min.without_tax price.retail_price_range.max.without_tax}}
<div class="price-section price-section--withTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
<span data-product-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}}
<meta itemprop="availability" content="{{product.availability}}">
<meta itemprop="itemCondition" itemtype="http://schema.org/OfferItemCondition" content="http://schema.org/{{product.condition}}Condition">
<div itemprop="priceSpecification" itemscope itemtype="http://schema.org/PriceSpecification">
<meta itemprop="minPrice" content="{{price.retail_price_range.min.without_tax.value}}" />
<meta itemprop="price" content="{{price.retail_price_range.min.without_tax.value}}">
<meta itemprop="maxPrice" content="{{price.retail_price_range.max.without_tax.value}}" />
<meta itemprop="priceCurrency" content="{{currency_selector.active_currency_code}}">
<meta itemprop="valueAddedTaxIncluded" content="true">
</div>
{{/if}}
{{#and price.retail_price_range.min.with_tax price.retail_price_range.max.with_tax}}
<div class="price-section price-section--withTax rrp-price--withTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
{{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}}
{{#if price.with_tax}}
<div class="price-section price-section--withTax rrp-price--withTax" style="display: none;">
{{theme_settings.pdp-retail-price-label}}
<span data-product-rrp-with-tax class="price price--rrp">
{{price.rrp_with_tax.formatted}}
</span>
</div>
{{/if}}
{{/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;">
{{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}}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

</span>
</div>
<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<span data-product-price-with-tax class="price">{{price.price_range.min.with_tax.formatted}} - {{price.price_range.max.with_tax.formatted}}</span>
{{#and price.price_range.min.without_tax price.price_range.max.without_tax}}
<abbr title="{{lang 'products.including_tax'}}">{{lang 'products.price_with_tax' tax_label=price.price_range.min.tax_label}}</abbr>
Expand All @@ -33,8 +40,31 @@
{{/if}}
</div>
{{/and}}
{{#and price.retail_price_range.min.without_tax price.retail_price_range.max.without_tax}}

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

<div class="price-section price-section--withoutTax rrp-price--withoutTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
{{theme_settings.pdp-retail-price-label}}
<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>
</div>
{{else}}
{{#if price.without_tax}}
<div class="price-section price-section--withoutTax rrp-price--withoutTax {{#if price.with_tax}}price-section--minor{{/if}}" style="display: none;">
{{theme_settings.pdp-retail-price-label}}
<span data-product-rrp-price-without-tax class="price price--rrp">
{{price.rrp_without_tax.formatted}}
</span>
</div>
{{/if}}
{{/and}}
{{#and price.price_range.min.without_tax price.price_range.max.without_tax}}
<div class="price-section price-section--withoutTax non-sale-price--withoutTax {{#if price.with_tax}}price-section--minor{{/if}}" style="display: none;">
{{theme_settings.pdp-non-sale-price-label}}
<span data-product-non-sale-price-without-tax class="price price--non-sale">
{{price.non_sale_price_without_tax.formatted}}
</span>
</div>
<div class="price-section price-section--withoutTax {{#and price_range.min.with_tax price_range.max.with_tax}}price-section--minor{{/and}}" {{#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>
<span data-product-price-without-tax class="price price--withoutTax">{{price.price_range.min.without_tax.formatted}} - {{price.price_range.max.without_tax.formatted}}</span>
{{#and price.price_range.min.with_tax price.price_range.max.with_tax}}
<abbr title="{{lang 'products.excluding_tax'}}">{{lang 'products.price_without_tax' tax_label=price.price_range.min.tax_label}}</abbr>
Expand Down
66 changes: 34 additions & 32 deletions templates/components/products/price.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,24 @@
{{> components/products/price-range price=price schema_org=schema_org}}
{{else}}
{{#if price.with_tax}}
<div class="price-section price-section--withTax rrp-price--withTax">
{{#if price.rrp_with_tax}}
{{lang 'products.retail_price'}}
<span data-product-rrp-with-tax class="price price--rrp"> {{price.rrp_with_tax.formatted}}</span>
{{/if}}
<div class="price-section price-section--withTax rrp-price--withTax" {{#unless price.rrp_with_tax}}style="display: none;"{{/unless}}>
{{theme_settings.pdp-retail-price-label}}
<span data-product-rrp-with-tax class="price price--rrp">
{{price.rrp_with_tax.formatted}}
</span>
</div>
<div class="price-section price-section--withTax non-sale-price---withTax">
{{#if price.non_sale_price_with_tax}}
{{lang 'products.price_was'}}
<span data-product-non-sale-price-with-tax class="price price--rrp"> {{price.non_sale_price_with_tax.formatted}}</span>
{{/if}}
<div class="price-section price-section--withTax non-sale-price--withTax" {{#unless price.non_sale_price_with_tax}}style="display: none;"{{/unless}}>
{{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}}
</span>
</div>
<div {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
<span class="price-now-label">
{{#if price.non_sale_price_with_tax}}
{{lang 'products.price_now'}}
{{/if}}
<div class="price-section price-section--withTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
<span class="price-label" {{#if price.non_sale_price_with_tax}}style="display: none;"{{/if}}>
{{theme_settings.pdp-price-label}}
</span>
<span class="price-now-label" {{#unless price.non_sale_price_with_tax}}style="display: none;"{{/unless}}>
{{theme_settings.pdp-sale-price-label}}
</span>
<span data-product-price-with-tax class="price"> {{price.with_tax.formatted}}</span>
{{#if schema_org}}
Expand All @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{{/if}}
</div>
{{/if}}
{{#if price.without_tax}}
<div class="price-section rrp-price--withoutTax price-section--withoutTax {{#if price.with_tax}}price-section--minor{{/if}}">
{{#if price.rrp_without_tax}}
{{lang 'products.retail_price'}}
<span data-product-rrp-price-without-tax class="price price--rrp"> {{price.rrp_without_tax.formatted}}</span>
{{/if}}
<div class="price-section price-section--withoutTax rrp-price--withoutTax {{#if price.with_tax}}price-section--minor{{/if}}" {{#unless price.rrp_without_tax}}style="display: none;"{{/unless}}>
{{theme_settings.pdp-retail-price-label}}
<span data-product-rrp-price-without-tax class="price price--rrp">
{{price.rrp_without_tax.formatted}}
</span>
</div>
<div class="price-section non-sale-price---withoutTax price-section--withoutTax {{#if price.with_tax}}price-section--minor{{/if}}">
{{#if price.non_sale_price_without_tax}}
{{lang 'products.price_was'}}
<span data-product-non-sale-price-without-tax class="price price--rrp"> {{price.non_sale_price_without_tax.formatted}}</span>
{{/if}}
<div class="price-section price-section--withoutTax non-sale-price--withoutTax {{#if price.with_tax}}price-section--minor{{/if}}" {{#unless price.non_sale_price_without_tax}}style="display: none;"{{/unless}}>
{{theme_settings.pdp-non-sale-price-label}}
<span data-product-non-sale-price-without-tax class="price price--non-sale">
{{price.non_sale_price_without_tax.formatted}}
</span>
</div>
<div {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
<span class="price-now-label">
{{#if price.non_sale_price_without_tax}}
{{lang 'products.price_now'}}
{{/if}}
<div class="price-section price-section--withoutTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}>
<span class="price-label" {{#if price.non_sale_price_without_tax}}style="display: none;"{{/if}}>
{{theme_settings.pdp-price-label}}
</span>
<span class="price-now-label" {{#unless price.non_sale_price_without_tax}}style="display: none;"{{/unless}}>
{{theme_settings.pdp-sale-price-label}}
</span>
<span data-product-price-without-tax class="price price--withoutTax"> {{price.without_tax.formatted}}</span>
<span data-product-price-without-tax class="price"> {{price.without_tax.formatted}}</span>
{{#if schema_org}}
<meta itemprop="availability" itemtype="http://schema.org/ItemAvailability"
content="http://schema.org/{{#if product.pre_order}}PreOrder{{else if product.out_of_stock}}OutOfStock{{else if product.can_purchase '===' false}}OutOfStock{{else}}InStock{{/if}}">
Expand Down