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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 amp-ima-video: Show sound and fullscreen buttons during ads #18954

Merged
merged 5 commits into from Nov 20, 2018

Conversation

Projects
None yet
6 participants
@curseagain
Copy link
Member

curseagain commented Oct 25, 2018

Closes #18533

Changes

First Commit

  • Controls are always visible while ads are playing.
  • The only buttons displayed in ad control panel are #ima-mute-unmute and #ima-fullscreen.
  • Controls are restored after the ad finishes.

Second Commit

  • The ad countdown timer has been moved to the control panel, is displayed when the ad controls are displayed and hidden when the ad controls are hidden

Third Commit

  • The ad countdown timer text can be customized by setting data-ad-label attribute to a pattern (e.g. data-ad-label="Publicidad (%s de %s)", resulting in something like "Publicidad (1 de 2): 0:27")
  • Added additional amp-ima-video test for ad with skip button
  • Note: The height of the ad label is fixed and any overflow is hidden

Fourth Commit

  • Display smaller controls when an ad is skippable and has mobile classes, signifying that the "Skip" button will be displayed and we will have to squish our controls underneath.
  • Fix bug that caused ad to be videoWidth instead of fullscreenWidth when fullscreen is toggled

Screenshots

Desktop with Custom data-ad-label


Desktop with Skip Button


Mobile without Skip Button


Mobile with Skip Button

@googlebot googlebot added the cla: yes label Oct 25, 2018

@curseagain

This comment has been minimized.

Copy link
Member Author

curseagain commented Oct 25, 2018

@torch2424 this is ready for review! However, I am concerned that the .videoAdUiAttribution element within the ad iframe is not clearly visible underneath the controls. See the screenshot in this PR. What do you think? Is that an ad disclosure that we aren't allowed to cover?

@torch2424 torch2424 self-requested a review Oct 25, 2018

@torch2424 torch2424 self-assigned this Oct 25, 2018

@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Oct 25, 2018

Wow this was fast! Awesome, thank you very much for this! 馃槃 Will look over this, and reach out to some people on our end.

I'll also ask around the for the .videoAdUiAttribution element for you 馃憤

cc @jasti

@torch2424
Copy link
Member

torch2424 left a comment

Code looks great! Great work! Very clean and concise 馃槃 And thank you for attaching a gif! It is super helpful!

Requesting changes, because I see in the Travis you are failing our lint:

https://travis-ci.org/ampproject/amphtml/jobs/445958589

If you can, run these commands to check all of the "pre-commit" things that may fail in Travis:

gulp lint --local-changes --fix

gulp check-types --local-changes

gulp presubmit --local-changes

gulp test --local-changes

Once those all pass, commit up your changes, and then I can approve once we get some more eyes on this from other teams

Thank you! 馃槃

@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Oct 25, 2018

Also, did some quick research, was suggested by @aghassemi that following the VideoJS IMA Plugin would be a great place to start.

Looking at their simple example It appears that their .videoAdUiAttribution is brought above the controls shade. So I think we should go ahead and do the same.

Tried looking around for more reference, but couldn't find much. Hopefully will know more later today 馃槃

@curseagain

This comment has been minimized.

Copy link
Member Author

curseagain commented Oct 25, 2018

@torch2424, it looks like the videojs team has created their own countdown timer to replace the default one, as described here.

That works fine, but with one complication: internationalization! The videojs player accepts custom values for adLabel and adLabelNofN, which we could potentially do as well... However, maybe there are similar translations happening elsewhere that we could refer to?

@curseagain curseagain force-pushed the curseagain:amp-ima-video-ad-controls-18533 branch from 471f888 to 8145b40 Oct 26, 2018

@curseagain

This comment has been minimized.

Copy link
Member Author

curseagain commented Oct 26, 2018

@torch2424 I've added a second commit in which I do some linting and attempt to add an ad countdown timer like they do in the videojs plugin. I've also attached a screenshot and updated the description in the PR to reflect the current problem: translation of the ad disclosure text.

@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Oct 26, 2018

@curseagain Thanks for all the videos! Really helpful!

Glanced over the code again, really like they way you implemented. Once we get finalized on an implementation I'll give a more thorough review (though I don't think I'll have too many comments). 馃槃

So, the Ads person we work with, is currently Out of Office until next week I think, so I sent them and email and then they can help advise us on this.

Thanks for making that countdown timer, and looking into that! Looks great!

i18n will definitely be a problem, and thank you very much for considering that. Once we hear back from our Ads contact, hopefully we can learn if there is something to do that for us. If not, we can discuss a strategy to move forward on i18n.

@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Oct 30, 2018

@curseagain

Just a quick update:

Got a response from the IMA team, but they sent me to ask someone else about the video controls and i18n haha! So now we will wait for that 馃槃

@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Oct 30, 2018

@curseagain

Got a super quick response from the IMA team 馃槃

"I would definitely hide when not hover. Other than that it looks fine for the most part. I would also try to minimize the shadow size of the controlbar so as to not interfere with the skip button and things like that".

So, once we get those implemented, I think we can pull that in 馃槃 We can either do the "show controls on hover" in another PR, merge to master, pull into this PR, and then go from there. Or I wouldn't mind just adding it to this PR.

I'm kind of low on bandwidth this sprint, so if you have time definitely feel free to get started on this. If you cannot, perhaps sometime next week, I can open a PR for the controls hover 馃槃

Also, just asked about i18n as well.

i18n is handled by the SDK, but does not expose any text for us. Therefore, we need to roll our own solution for this. I'll ask around on my team to see if anyone has a good idea 馃槃

Thanks!

@curseagain

This comment has been minimized.

Copy link
Member Author

curseagain commented Oct 30, 2018

Thanks @torch2424! I can begin on some of the other tweaks while we wait to know more about i18n.

@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Oct 31, 2018

@curseagain

So I brought up i18n at Today's design review #18658 (Thank you so much for bringing it up in the issue though, totally forgot that it was a good place to discuss there 馃檭)

They said we could make i18n an attribute on the component itself! They said for consistency's sake, we could/should follow what amp-carousel is doing with their data-button-count-format.

We can simply pass in that attribute value into the ima video player like we are the other data- attributes, and provide a fallback if they don't provide a label. See the amp-carousel code to see what their fallback is, and we should just follow that pattern.

I suggest we name this label: data-ad-label, or whatever would be most consistent with IMA Sdk. Whenever we implement the i18n in this PR, or another, just provide a short paragraph with some links on the naming decision and we can change it if needed.

Thanks! 馃槃

@curseagain

This comment has been minimized.

Copy link
Member Author

curseagain commented Nov 15, 2018

@torch2424, I've committed the change to accept data-ad-label attribute as a pattern, similar to the way it's done in amp-carousel. I've also updated the description in the PR so you can see the ad controls in different contexts.

Potential problem at mobile sizes where ad controls and the skip button are very close to each other? Is it fine as-is? Or do we need to reduce the height and font size of the controls a bit more?

Thanks for your review and feedback!!

@curseagain

This comment has been minimized.

Copy link
Member Author

curseagain commented Nov 15, 2018

I will update documentation as well after our changes are approved.

Also, let's do "show controls on hover" in a follow-up that you can assign to me, if that's okay?

@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Nov 15, 2018

@curseagain Thank you very much for making that change it is very much appreciated! 馃槃

Thanks for adding Gifs, the previews definitely help understand what's going on 馃槃

On mobile that does look a bit odd, if possible, could we reduce the size of the height of the controls a little bit more?

And updating docs and "show controls on hover" sounds good to be in a follow up PR to me. Is it alright to finish up this feature in another PR @aghassemi ?

Doing a review now, thanks again for your work! 馃槃

@torch2424
Copy link
Member

torch2424 left a comment

Great work!

Just a few small questions, and asked for some help from @zhouyx for if we can use the innerHTML, other than that, mostly compliments haha!

Thank you!

@@ -248,17 +257,38 @@ export function imaVideo(global, data) {
'bottom': '0px',
'width': '100%',
'height': '100px',
'background-color': 'rgba(7, 20, 30, .7)',
'background':
'linear-gradient(0, rgba(7, 20, 30, .7) 0%, rgba(7, 20, 30, 0) 100%)',

This comment has been minimized.

@torch2424

torch2424 Nov 15, 2018

Member

Ahhh I like this approach, thank you! 馃槃


<h2>Video4</h2>
<div class="description">
Uses the Single Skippable Inline IMA Sample Tag. Also, this video with autoplay

This comment has been minimized.

@torch2424

torch2424 Nov 15, 2018

Member

Great Example, thank you!

@@ -210,6 +218,7 @@ export function imaVideo(global, data) {

videoWidth = global./*OK*/innerWidth;
videoHeight = global./*OK*/innerHeight;
adLabel = data.adLabel || 'Ad (%s of %s)';

This comment has been minimized.

@torch2424

torch2424 Nov 15, 2018

Member

Let's make sure to document later on that this will be the default 馃憤

Show resolved Hide resolved ads/google/imaVideo.js Outdated
Show resolved Hide resolved ads/google/imaVideo.js Outdated
@aghassemi

This comment has been minimized.

Copy link
Member

aghassemi commented Nov 19, 2018

Sorry for late reply: "show controls on hover" as a feature in a later PR would be great.

@curseagain

This comment has been minimized.

Copy link
Member Author

curseagain commented Nov 19, 2018

@torch2424 I've made the changes that were requested, described them in the Pull Request Description, and added some gifs. Thanks!

@torch2424
Copy link
Member

torch2424 left a comment

Code Looks good, all requested changes have been made 馃槃

Thank you so much @curseagain for your contribution! We really appreciate it! I'll set up a tracking issue for the remaining work, and assign to both of us. Feel free to open up a PR for that as well, and I'll be glad to review it and get it pulled in. Thank you!

Also, did some manual testing in all major browsers and everything looks great! Here's a huge screenshot dump.

screen shot 2018-11-19 at 3 58 30 pm

screen shot 2018-11-19 at 3 59 34 pm

screen shot 2018-11-19 at 3 59 39 pm

screen shot 2018-11-19 at 3 59 51 pm

screen shot 2018-11-19 at 4 00 31 pm

screen shot 2018-11-19 at 4 00 41 pm

screen shot 2018-11-19 at 4 00 56 pm

screen shot 2018-11-19 at 4 01 33 pm

screen shot 2018-11-19 at 4 01 38 pm

screen shot 2018-11-19 at 4 01 45 pm

screen shot 2018-11-19 at 4 02 04 pm

@torch2424 torch2424 merged commit 6b09d02 into ampproject:master Nov 20, 2018

6 checks passed

LGTM analysis: JavaScript No alert changes
Details
LGTM analysis: Python No code changes detected
Details
ampproject/owners-bot approvals met: false. lannka, zhouyx, ampproject/a4a, aghassemi, alanorozco, torch2424
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details
@torch2424

This comment has been minimized.

Copy link
Member

torch2424 commented Nov 20, 2018

Opened up a tracking issue for the remaining work for the newly introduced features: #19393

cc @curseagain

Enriqe added a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018

馃悰 amp-ima-video: Show sound and fullscreen buttons during ads (amppro鈥
鈥ect#18954)

* amp-ima-video: Show sound and fullscreen buttons during ads

* add #ima-countdown to replace ad counter

* use data-ad-label to support custom ad countdown label for i18n

* smaller controls for ads that are mobile&skippable, fix fullscreen bug

* Set font-family in controlsDiv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.