-
Notifications
You must be signed in to change notification settings - Fork 606
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
fix(storefront): STRF-2456 Fixing Schema Org meta tags #1233
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
@mattolson @Ubersmake @Souldjinn @mjschock This is ready for review |
CHANGELOG.md
Outdated
@@ -12,6 +12,7 @@ | |||
- Fix for excess whitespace in multiline text field product option [#1222](https://github.com/bigcommerce/cornerstone/pull/1222) | |||
- Fix for faceted search display. [#1225](https://github.com/bigcommerce/cornerstone/pull/1225) | |||
- Fix for calls with empty files in Safari. [#1210](https://github.com/bigcommerce/cornerstone/pull/1210) | |||
- Fix product pricing schema.org microdata. [#1233](https://github.com/bigcommerce/cornerstone/pull/1233) |
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.
This needs to go into the Draft section.
@@ -1,5 +1,5 @@ | |||
{{#and price_range.min.with_tax price_range.max.with_tax}} | |||
<div class="price-section price-section--withTax" {{#if schema_org}}itemprop="offers" itemscope itemtype="http://schema.org/Offer"{{/if}}> |
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.
Why is this removed? We don't care about the withTax case? Wouldn't you need to add a div below for the microdata to be understood, or are you just orphaning that data?
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.
I believe the div with class can stay however the schema meta needs to be removed. Will fix.
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.
I don't understand. This is embedded in templates/components/products/price.html
without any "offers" wrapper so why is it ok to remove this? If you are removing it here, should you add the wrapper div below?
d84e86e
to
0892688
Compare
Much slimmer changeset. |
<abbr title="{{lang 'products.excluding_tax'}}">{{lang 'products.price_with_tax' tax_label=price_range.min.tax_label}}</abbr> | ||
{{else if schema_org}} | ||
<abbr title="{{lang 'products.including_tax'}}">{{lang 'products.price_with_tax' tax_label=price_range.min.tax_label}}</abbr> | ||
|
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.
Extra line
<abbr title="{{lang 'products.including_tax'}}">{{lang 'products.price_with_tax' tax_label=price_range.min.tax_label}}</abbr> | ||
|
||
{{/and}} | ||
{{#if schema_org}} |
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.
Extra indent
@@ -13,7 +15,7 @@ | |||
<meta itemprop="priceCurrency" content="{{currency_selector.active_currency_code}}"> | |||
<meta itemprop="valueAddedTaxIncluded" content="true"> | |||
</div> | |||
{{/and}} | |||
{{/if}} |
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.
Extra indent
@@ -28,6 +29,7 @@ <h2 class="productView-brand"{{#if schema}} itemprop="brand" itemscope itemtype= | |||
{{/or}} | |||
</div> | |||
{{{region name="product_below_price"}}} | |||
{{#if product.rating}} |
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.
This seems unrelated to the change. Is there another bug you are trying to fix?
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.
yes we were getting rating sections showing up on products without ratings
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.
Ok, with the introduction of this new block, please indent the contents
<h2 class="productView-brand"{{#if schema}} itemprop="brand" itemscope itemtype="http://schema.org/Brand"{{/if}}> | ||
<a href="{{product.brand.url}}"{{#if schema}} itemprop="url"{{/if}}><span{{#if schema}} itemprop="name"{{/if}}>{{product.brand.name}}</span></a> | ||
</h2> | ||
{{#if product.brand}} |
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.
This seems unrelated to the change. Is there another bug you are trying to fix?
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.
yes. we were getting brand sections showing up on products without brands.
b15e84b
to
ec64daa
Compare
…and moving some tags to the appropriate div
What?
We are adding schema.org offer tags on div's that schema.org doesn't care about what price was
it only cares about what price currently is afaict I need to validate still that price-range schema works.
Tickets / Documentation
Screenshots
Fixed product schema with "was" and "currently" prices