-
Notifications
You must be signed in to change notification settings - Fork 16
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
Refactor repeated prose regarding the specification of style property… #526
Conversation
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.
In general I like the change. I'm just wondering why the text was moved from 10.2 "Styling Attribute Vocabulary" to 10 "Styling", when it only talks about attributes.
in order to cover 10.3 as well |
@skynavga indeed, makes sense then. |
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.
Minor fixes requested.
spec/ttml2.xml
Outdated
<p>Unless explicitly stated otherwise, linear white-space (LWSP) must | ||
appear between adjacent non-terminal components of a value of a TT Audio Style property | ||
value unless some other delimiter is permitted and used, in which case linear white-space may appear | ||
between a delimiter and a non-terminal component.</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.
This sensible change appears to be in the wrong pull request.
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.
spec/ttml2.xml
Outdated
@@ -7782,6 +7782,14 @@ to implement features it has in common with this model.</p> | |||
<p>No normative use of an | |||
<code><?xml-stylesheet ... ?></code> processing instruction is defined | |||
by this specification.</p> | |||
<p>The styling attributes defined in this section may be specified by any element type | |||
that permits use of attributes in the TT Style Namespaces; however, these attributes apply |
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 need to be explicit that this applies to both the TT Style Namespace and the TT Audio Style Namespace.
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.
Perhaps we could resolve the possible confusion by wrapping "TT Style Namespaces" in a <loc>
element that points to the place it is defined?
On Tue, Jan 2, 2018 at 4:38 PM, Nigel Megitt ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Minor fixes requested.
------------------------------
In spec/ttml2.xml
<#526 (comment)>:
> @@ -14470,15 +14287,15 @@ that support inline style specifications:</p>
<item><p><specref ref="audio-style-attribute-voice"/></p></item>
-->
</ulist>
-
+<p>Unless explicitly stated otherwise, linear white-space (LWSP) must
+appear between adjacent non-terminal components of a value of a TT Audio Style property
+value unless some other delimiter is permitted and used, in which case linear white-space may appear
+between a delimiter and a non-terminal component.</p>
This sensible change appears to be in the wrong pull request.
------------------------------
In spec/ttml2.xml
<#526 (comment)>:
> @@ -7782,6 +7782,14 @@ to implement features it has in common with this model.</p>
<p>No normative use of an
<code><?xml-stylesheet ... ?></code> processing instruction is defined
by this specification.</p>
+<p>The styling attributes defined in this section may be specified by any element type
+that permits use of attributes in the TT Style Namespaces; however, these attributes apply
I think we need to be explicit that this applies to both the TT Style
Namespace and the TT Audio Style Namespace.
It is. Note the 's' on "TT Style Namespaces" above.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#526 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAXCb_TtmPxQy0prZ21iz8FVUKYoKes-ks5tGr3ogaJpZM4RPsvE>
.
|
@skynavga Sorry, but that wording is too subtle. I did see it, but I couldn't see anything that lists the "TT Style Namespaces" explicitly anywhere. |
On Tue, Jan 2, 2018 at 5:08 PM, Nigel Megitt ***@***.***> wrote:
It is. Note the 's' on "TT Style Namespaces" above.
@skynavga <https://github.com/skynavga> Sorry, but that wording is too
subtle. I did see it, but I couldn't see anything that lists the "TT Style
Namespaces" explicitly anywhere.
See 2nd para under table 5-1.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#526 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAXCb4dixOlx8fHg703XVyGWjzcpP1V3ks5tGsULgaJpZM4RPsvE>
.
|
@skynavga thanks, I hadn't spotted that. |
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.
Thanks! This addresses my comment made in #526. Although I approve the PR as is, I would fully support @nigelmegitt to name the Audio Style Namespace explicitly. I came to the same misunderstanding and wanted to open an WR issue until I noticed the plural form. If @nigelmegitt and me came first to the wrong conclusion other readers may also have the same problem.
[Note to Self] Add link to inline definition of TT Style Namespaces. |
@nigelmegitt please review fixes in c46fe72 |
Resolved conflicts.
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.
These changes work for me, thank you. @skynavga please double check that my conflict resolution matches your expectations.
|
||
<!-- termdef: sentence or phrase defining a term --> | ||
<xsl:template match="termdef"> | ||
<a id="{@id}" title="{@term}"> |
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'm not sure why but the latest changes did not trigger a new travis build, so the effect of this change is not visible on the version served by gh-pages... I've merged from master and resolved conflicts so hopefully that will trigger the build again.
Latest Changes work for me. |
… attributes (#438).
Closes #438.