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

Video main media caption #15952

Merged
merged 2 commits into from Mar 8, 2017
Merged

Video main media caption #15952

merged 2 commits into from Mar 8, 2017

Conversation

zeftilldeath
Copy link

@zeftilldeath zeftilldeath commented Feb 24, 2017

Currently, within the new-header test we're getting some horrible banding due to the placement of captions under video-main-media. If this becomes permanent we'll need to support youtube videos too.

This will not do

screen shot 2017-02-24 at 15 57 30

This will do

screen shot 2017-02-24 at 15 55 47

When a video is main media AND has a caption AND there is a feature tone, you get this horrible white banding under the image to make room for the caption. Instead with a lot of help from @NataliaLKB I have used the caption toggle just like images that are main media.

@PRBuilds
Copy link

PR build results:

screenshots
mobile.pngwide.pngtablet.pngdesktop.png

exceptions (0)
thrown-exceptions.js

webpagetest (0)
performanceComparisonSummary.txt

-automated message

@fragments.inlineSvg("information", "icon", List("rounded-icon", "centered-icon"))
@Html(caption)
</figcaption>
@if(mvt.ABNewNavVariantSeven.isParticipating) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, is this solely an issue for people in this A/B test? If so would have been helpful to put this in the description.

// These events are so that other libraries (e.g. Ophan) can hook into events without
// needing to know about videojs
function bindGlobalEvents(player) {
player.on('playing', function () {
kruxTracking(player, 'videoPlaying');
bean.fire(document.body, 'videoPlaying');
hideCaption();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you doing this all the time or only when the video plays

@gidsg
Copy link
Contributor

gidsg commented Mar 6, 2017

It would be worth testing this same scenario with YouTube atoms, as I think they would need to be updated separately, though this can be done in a separate PR.

@gidsg
Copy link
Contributor

gidsg commented Mar 6, 2017

Would be helpful for me to talk with @zeftilldeath or @NataliaLKB to provide a bit more context if possible.

@NataliaLKB
Copy link
Contributor

@gidsg Best to chat to @zeftilldeath about this! I am in NY until the 14th. But after the 14th I am free to chat 😄

@gidsg
Copy link
Contributor

gidsg commented Mar 8, 2017

Talked through IRL with @zeftilldeath looks good to me 👍 🎥

@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @zeftilldeath 16 minutes and 44 seconds ago) Please check your changes!

@zeftilldeath zeftilldeath deleted the video-main-media-caption branch March 8, 2017 11:00
@NataliaLKB NataliaLKB restored the video-main-media-caption branch March 27, 2017 10:45
@NataliaLKB NataliaLKB deleted the video-main-media-caption branch June 16, 2017 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants