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

Improve overflow example #328

Merged
merged 6 commits into from
Jan 29, 2018
Merged

Conversation

palemieux
Copy link
Contributor

Closes #320

@palemieux palemieux added this to the 3rd Ed milestone Jan 16, 2018
@palemieux palemieux self-assigned this Jan 16, 2018
Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Please could you add a validating step to build.xml to check the new example XML file?

Also requesting a colour change and less importantly a change to make the application of styles consistent.

<head>
<styling>
<style xml:id="s1" tts:backgroundColor="black" tts:wrapOption="noWrap" tts:padding="6px"/>
<style xml:id="s2" tts:color="red" tts:backgroundColor="purple" tts:fontFamily="proportionalSansSerif" tts:fontSize="10px"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm finding the colour contrast between the purple and the red really horrible (and the luminance contrast is very low too) - please could you change one of them, e.g. change the purple to grey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grey does not provide sufficient contrast with the background. Please pick another color.

Copy link
Contributor

Choose a reason for hiding this comment

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

White?

<tt xmlns="http://www.w3.org/ns/ttml" xmlns:tts="http://www.w3.org/ns/ttml#styling" xml:lang="en" tts:extent="320px 240px">
<head>
<styling>
<style xml:id="s1" tts:backgroundColor="black" tts:wrapOption="noWrap" tts:padding="6px"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why put tts:wrapOption in the region style? It seems inconsistent to do this but put tts:fontFamily and tts:fontSize in the p style - those three should probably all go on the same style, I don't mind which one.

Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Sorry, one more fix to make.

spec/build.xml Outdated
<include name="ex4-overflow.xml"/>
</fileset>
</schemavalidate>
</target>

<target name="validate-examples"
depends="validate-example-1, validate-example-2, validate-example-3"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add validate-example-4 to the dependencies here.

Copy link
Contributor

Choose a reason for hiding this comment

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

(also, separately, I see that the build target does not call validate first, which it should)

Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Thank you!

@palemieux palemieux merged commit 15a45c7 into master Jan 29, 2018
@skynavga skynavga deleted the issue-320-improve-overflow-example branch March 11, 2018 22: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.

2 participants