-
Notifications
You must be signed in to change notification settings - Fork 10
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
Editorial cleanup #54
Conversation
- differentiate required properties from canonical defaults; - fixes for language consistency between sections; - consolidation of prose; - spelling and grammar fixes; - diversification of examples
index.html
Outdated
<p>The digital publication manifest includes entries to set both these concepts — both <a | ||
href="#manifest-lang-dir-global">globally</a> and for <a href="#manifest-lang-dir-local" | ||
>individual items</a> — to aid user agents in interpreting and presenting the | ||
metadata.</p> |
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.
"both globally and for individual items": unfortunately, that is not correct for the base direction of individual items. At this point I think the best is to remove that side remark.
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.
Right, but I'd hate to lose the explanation. I'll change to clarify language in the subclause:
The digital publication manifest provides the ability to set both these concepts globally — and, for language, on individual items — to aid user agents in interpreting and presenting the metadata.
as JSON-LD does not currently include facilities for this. As an interim measure, this | ||
specification defines a <code>direction</code> property for this purpose, as described in the | ||
following table.</p> | ||
<p>Unlike the default language, the default text direction cannot be specified in the context as |
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.
"The global language declaration MUST follow the..." somehow it took me a while to understand what was said that there. Maybe it would help to put in a link to the "publication context" term definition rather than the reference to the section. (Ie, introduce 'publication context' with a <def>
tag, or something of that effect)
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.
Ya, the dangling modifier at the end doesn't help, either, as it's the language that isn't required. The publication context always has to be present.
index.html
Outdated
manifests to include this.</p> | ||
|
||
<p>For more information about localized strings on the Web, refer to [[string-meta]].</p> | ||
</div> | ||
|
||
<p>In order to correctly handle manifests entries containing right-to-left or bidirectional text, | ||
user agents SHOULD identify the base direction of any given natural language value by scanning |
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 wonder whether, editorially, it is not better to move this set of paragraphs and notes into a separate appendix, possibly appendix D. It breaks the natural flow of the text at this point, going into much (necessary) details. After all, per the note above, these remarks may become obsolete if the direction is properly handled in RDF/JSON-LD at some points later, making this text a bit unnecessary.
<pre class="example" title="Reading progression set explicitl to ltr"> | ||
{ | ||
"@context" : ["https://schema.org","https://www.w3.org/ns/pub-context"], | ||
"type" : "Book", | ||
… | ||
"url" : "https://publisher.example.org/mobydick", | ||
"url" : "https://publisher.example.org/leviathan", | ||
"readingProgression" : "ltr" | ||
} | ||
</pre> |
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 wonder whether it is worth adding a note whereby later versions of this specifications may add more values to this property, like "down" or "up" for certain type of publications. Maybe @llemeurfr can advise on this (or adding it already now?)
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, let's take this up in #51, though. I'm fine adding another value now if it's reasonable that a top-to-bottom progression will be needed, otherwise we need to clarify extensibility as right now the wording wouldn't allow for other values.
well-formed [[!bcp47]] and issue a warning for any invalid values found.</p> | ||
Recursively check that every <var>language</var> property in <var>processed</var> is <a | ||
href="https://tools.ietf.org/html/bcp47#section-2.2.9">valid</a> [[!bcp47]] and | ||
issue a warning for any invalid values found.</p> | ||
</li> | ||
|
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 think we should keep to the 'well-formed' for now (that is used in other parts of the spec). The response to the 'valid' question should come from the I18N guys, and, afaik, @r12a will raise an issue about it in an I18N repository. (We may want to add an editorial note here, just that we do not forget about it...)
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.
Oops, I thought I undid them all! I was intending to leave it alone pending a resolution. I'll run another search on valid and add a note.
invalid values found.</p> | ||
that each value in its array is <a | ||
href="https://tools.ietf.org/html/bcp47#section-2.2.9">valid</a> [[!bcp47]] and | ||
issue a warning for any invalid values found.</p> | ||
</li> |
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.
See my comments above about 'valid'
This PR is fairly large, but the bulk of the changes are purely editorial fixes. These include:
The one change of note is to the required manifest properties. I noticed that we still listed title and reading order, but these are not required in the manifest. They are required in the canonical representation, as they always get compiled if not explicit. I've done my best to make the distinction between the manifest requirements and the WebIDL clear, but comments welcome. (See the changes to 2.2 and 2.3.)
Preview | Diff