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

Fix to HowTo examples #1895

Merged
merged 6 commits into from
Apr 26, 2018
Merged

Fix to HowTo examples #1895

merged 6 commits into from
Apr 26, 2018

Conversation

RichardWallis
Copy link
Contributor

Implements fix for issue (#1884)

Dataliberate and others added 4 commits April 24, 2018 09:50
… 'paymentAccepted' & 'currenciesAccepted' properties.

For consistency, also added to description of properties: 'discountCurrency', 'price', 'priceCurrency', 'currency'.
Reversed some changes not needed as already visible on webschemas.org eg. HowToSection now a subtype of ListItem, HowToDirection now a subtype of CreativeWork.
@danbri
Copy link
Contributor

danbri commented Apr 25, 2018

Thanks, though not gonna rush this one. There are unconventional currencies other than "crypto" currencies, https://en.wikipedia.org/wiki/Local_exchange_trading_system#Examples that we maybe ought to handle in common manner.

@danbri
Copy link
Contributor

danbri commented Apr 25, 2018

Actually my comment was meant w.r.t. #1866 "Added reference to cryptocurrencies in description" ; not sure what those edits were doing here, was that part of the bit you removed?

Removing references to issue (#1866) incorrectly added to branch 1884
This reverts commit fc4ea93.
…tion for 'paymentAccepted' & 'currenciesAccepted' properties."

This reverts commit 182dd79.
@RichardWallis
Copy link
Contributor Author

Spurious commits from cryptocurrencies work reverted. This PR now only includes changes/enhancements to HowTo examples. Good to go for a merge!

@danbri
Copy link
Contributor

danbri commented Apr 26, 2018

Looking at this with a colleague, we are concerned the steps section is broken, but will look at a followup edit after merging as the other fixes are good.

@danbri danbri merged commit b7cd388 into master Apr 26, 2018
</span>$20</div>
<div>About <span itemprop="totalTime" content="PT30M">30 minutes</span></div>
<div>Necessary Items:</div>
<div itemprop="tool" itemscope itemtype="http://schema.org/HowToTool">Spare tire</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the name of the HowToTool will not be set with this syntax. The previous way works, or if you remove the itemscope & itemtype.

@RichardWallis
Copy link
Contributor Author

RichardWallis commented Apr 27, 2018

@dbiollo Good point.

There are two approaches to getting the name of the HowToTool, and HowToSuppy, to be picked out.


```
<div itemprop="tool" >Spare tire</div>
```
or
```
<div itemprop="tool" itemscope itemtype="http://schema.org/HowToTool">
     <span itemprop="name">Spare tire</span>
</div>
```
My preference is for the second of these as it could be expanded thus:
```
<div itemprop="tool" itemscope itemtype="http://schema.org/HowToTool">
     <span itemprop="name">Spare tire</span>
     <img itemprop="image" src="spare-tire.jpg" />
</div>
```
I will raise a new issue to explore this fix and the concerns raised by @danbri 

Update: New issue raised: (#1899)

</div>
<strong><span itemprop="name">Change a Flat Tire</span></strong>
<div>About <span itemprop="estimatedCost" itemscope itemtype="http://schema.org/MonetaryAmount">
<meta itemprop="currency" content="USD"></meta>
Copy link
Contributor

@dbiollo dbiollo Apr 27, 2018

Choose a reason for hiding this comment

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

There isn't a close tag, meta is an empty element.

<div itemprop="tool" itemscope itemtype="http://schema.org/HowToTool">Jack</div>
<div itemprop="tool" itemscope itemtype="http://schema.org/HowToTool">Wheel wedges</div>
<div itemprop="supply" itemscope itemtype="http://schema.org/HowToSupply">Flares</div>
<div itemprop="steps" itemscope itemtype="http://schema.org/ItemList">
Copy link
Contributor

@dbiollo dbiollo Apr 27, 2018

Choose a reason for hiding this comment

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

The field name is now called "step" - see http://webschemas.org/HowTo

Copy link
Contributor

Choose a reason for hiding this comment

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

Also "step" can be CreativeWork, HowToSection, HowToStep, Text. So I don't think ItemList is valid is it?

If the ItemList wrapper is removed then the sections can be steps like so:

<div itemprop="step" itemscope itemtype="http://schema.org/HowToSection">

<meta itemprop="position" content="2"></meta>
<div itemprop="text">You're going to need space and want to be visible.</div>
</div>
<div itemprop="itemListElement" itemscope itemtype="http://schema.org/HowToDirection">
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this changes the meaning of the HowTo Steps. Originally there was a HowToStep wrapping each HowToDirection. My understanding is that a HowToStep represents a (usually numbered) significant step, which can contain the HowToDirection primary text and optionally a HowToTip (a side note for more complex steps).

Was there any intent / discussion around changing the design of the HowTo for this release? If not, I'd suggest keeping the original HowToStep structure (wrapping the direction + tip) for now.

<div itemprop="itemListElement" itemscope itemtype="http://schema.org/HowToSection">
<div itemprop="name">Preparation</div>
<meta itemprop="position" content="1"></meta>
<div itemprop="steps" itemscope itemtype="http://schema.org/HowToStep">
Copy link
Contributor

Choose a reason for hiding this comment

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

HowToSection does not have a "steps" property, but it is an ItemList so this itemprop should be "itemListElement".

@dbiollo
Copy link
Contributor

dbiollo commented Apr 27, 2018

Took a closer look and left some comments on things that look problematic.

@RichardWallis
Copy link
Contributor Author

Changes made and now in PR (#1902)

@RichardWallis RichardWallis deleted the fix1884 branch February 15, 2019 14:49
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.

None yet

4 participants