-
Notifications
You must be signed in to change notification settings - Fork 25
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
Close control fixes. #21
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.
needs another pass to clean up language completely - "since video ad creative are expected" (should be is); "can dismiss of the video ad creative" (of not needed); etc
index.bs
Outdated
Also, since video ad creative are expected to adhere to industry guidelines that | ||
include a “Close X” button, the video player should not include its own close | ||
button for SIVIC ad units. | ||
This specification does not mandate a close control or behavior by the video ad |
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 suggest adding this as a sub section.
Close Button
Either the player or ad may render a close button but is not required to. Either can dismiss the ad at any point in time. When the ad is disposed by a close button the player should not fire VAST tracking pixels like complete.
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 sub section includes most of the points that have been added. I don't agree with the last statement. This doesn't belong in SIVIC.
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 am fine with this change. I think that the complete tracking pixel should not be fired when the user closes. It might not be necessary to specify it as it should be apparent.
@@ -992,57 +992,92 @@ | |||
.toc > li li { font-weight: normal; } | |||
.toc > li li li { font-size: 95%; } | |||
.toc > li li li li { font-size: 90%; } | |||
.toc > li li li li .secno { font-size: 85%; } |
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.
Do we want to have pull requests modify index.html? Seems like we shouldn't and bikeshed should handle.
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.
Yeah, that's partly why I opened #15 at the time.
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.
We would indeed move away from doing that once we have the proper tooling in place
Will go through and check grammar and add the additional clarification in a bit. |
@mtumi You were looking at most of the language that was removed as part of this pull request. I have fixed the one grammatical error that was contained (an extra of). |
Ready for another review. |
index.bs
Outdated
Also, since video ad creative are expected to adhere to industry guidelines that | ||
include a “Close X” button, the video player should not include its own close | ||
button for SIVIC ad units. | ||
This specification does not mandate a close control or behavior by the video ad |
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 am fine with this change. I think that the complete tracking pixel should not be fired when the user closes. It might not be necessary to specify it as it should be apparent.
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 will approve, assuming we'll get grammatical issues later on. There are still singular/plural issues. For example: "video ad creative are" (should be "video ad creative is") "video ad creatives may opt to show its" (should be "video ad creative should opt to show their") etc
Looking for feedback on this PR.