-
Notifications
You must be signed in to change notification settings - Fork 614
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
feat(catalog): CATALOG-3161 retail price range #1199
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
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.
Overall makes sense however you may have a conflict depending on when you get this merged. It will be regarding the itempprop="offers"
/ schema.org tags
@Souldjinn This needs a rebase after David's change |
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.
Schema Org section looks good too me. Some simple indent changes would be nice but not necessary.
<abbr title="{{lang 'products.including_tax'}}">{{lang 'products.price_with_tax' tax_label=price_range.min.tax_label}}</abbr> | ||
<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}}"> |
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.
Ident this block please.
and make it strike through.
What?
Show a retail price range if one exists and make it strike through.
This pull request is dependent on another, and will basically only show if all fo the variants to this product contain a retail price.
I didnt add the message that says "Without Tax" because retail price wont ever be calculated with tax anyway so its kind of redundant/misleading.
Tickets / Documentation
The below ticket must be finished before this can be merged
Screenshots (if appropriate)