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

鉁╝mp-ima-video: Show Controls on Hover #20130

Merged
merged 5 commits into from Jan 9, 2019

Conversation

Projects
None yet
4 participants
@torch2424
Copy link
Member

torch2424 commented Jan 2, 2019

relates to #19393

This enables showing ima-video controls on hover, instead of onclick. This also makes sure to work on mobile devices to preserve the tap to show controls behavior, as it is in amp-video.

From a UX perspective, got an approval by @spacedino to make amp-ima-video to match the behavior of amp-video as much as possible, and this PR falls inline with that 馃槃.

cc @curseagain

Example

Desktop

imavideohover

Mobile

imavideomobile

torch2424 added some commits Dec 29, 2018

@torch2424 torch2424 requested review from curseagain and zhouyx Jan 2, 2019

@googlebot googlebot added the cla: yes label Jan 2, 2019

@zhouyx
Copy link
Collaborator

zhouyx left a comment

Thanks for the new feature! Looks cool. LGTM with one nit and one question.

@@ -579,7 +607,7 @@ function onImaLoadSuccess(global, data) {
function onImaLoadFail() {
// Something blocked ima3.js from loading - ignore all IMA stuff and just play
// content.
videoPlayer.addEventListener(interactEvent, showControls);
addHoverEventToElement(/** @type !Element*/(videoPlayer), showControls);

This comment has been minimized.

@zhouyx

zhouyx Jan 3, 2019

Collaborator

nit: extra space after */

setStyle(controlsDiv, 'display', 'flex');
controlsVisible = true;
}

// Hide controls after 3 seconds
if (playerState == PlayerStates.PLAYING) {

This comment has been minimized.

@zhouyx

zhouyx Jan 3, 2019

Collaborator

I was playing with the mousemove event, and it seems that it fires a LOT. I am not sure about the cost of setTimeout and clearTimeout. Do you think it's fine w/o a throttle?

This comment has been minimized.

@torch2424

torch2424 Jan 3, 2019

Author Member

Yeah the mousemove event does fire a lot, but I was thinking about it and I essentially came to two conclusions:

  1. The event will only be fired on desktop devices, during certain sections of watching the video.
  2. Adding a throttle may improve user experience, however, I couldn't think of a good way to implement this that would be flexible and testable, thus hurting developer experience.

Though, I do think this is worth having a quick chat about on how we can easily add/remove this dynamic event, that has a throttle. 馃槃 I'll reach out tommorow, or vice/versa 馃憤

This comment has been minimized.

@curseagain

curseagain Jan 3, 2019

Member

Plus one for throttling - if possible - though controlsVisible is clear and succinct!

imaVideoObj.showControls
);
videoPlayerElement.dispatchEvent(clickEvent);

This comment was marked as resolved.

@curseagain

curseagain Jan 3, 2019

Member

could potentially add a test for mouseMoveEvent here to be sure that future changes don't break this cool feature

setStyle(controlsDiv, 'display', 'flex');
controlsVisible = true;
}

// Hide controls after 3 seconds
if (playerState == PlayerStates.PLAYING) {

This comment has been minimized.

@curseagain

curseagain Jan 3, 2019

Member

Plus one for throttling - if possible - though controlsVisible is clear and succinct!

}

/**
* Removess the appropriate event listener from an element

This comment was marked as resolved.

@curseagain

curseagain Jan 3, 2019

Member

typo Removess => Remove

@@ -507,6 +512,29 @@ export function imaVideo(global, data) {
}
}

/**
* Adds the appropriate event listener to an element
* to represent a hover state. And adds an appropriate

This comment was marked as resolved.

@curseagain

curseagain Jan 3, 2019

Member

No throttling here

torch2424 added some commits Jan 3, 2019

@torch2424

This comment has been minimized.

Copy link
Member Author

torch2424 commented Jan 4, 2019

@zhouyx @curseagain made all requested changes! Added a throttler as well 馃槃

@curseagain
Copy link
Member

curseagain left a comment

LGTM. Great tests!

* since this is applied per element,
* and would require wrapping the callback.
* Thus, the callback passed should be,
* appropriately throttled. See showControlsThrottled.

This comment has been minimized.

@curseagain

curseagain Jan 4, 2019

Member

Great commenting!! 鉂わ笍

This comment has been minimized.

@torch2424

torch2424 Jan 4, 2019

Author Member

Thanks! 馃槃

@torch2424

This comment has been minimized.

Copy link
Member Author

torch2424 commented Jan 4, 2019

@zhouyx This is good to go! Fixed the rate-limit build system thing 馃槃

@zhouyx

zhouyx approved these changes Jan 9, 2019

Copy link
Collaborator

zhouyx left a comment

Sorry for the late response here. LGTM. Thank you for the feature!!!

@torch2424

This comment has been minimized.

Copy link
Member Author

torch2424 commented Jan 9, 2019

@zhouyx No worries! Thank you!

@torch2424 torch2424 merged commit 4e6291a into ampproject:master Jan 9, 2019

5 checks passed

LGTM analysis: JavaScript No alert changes
Details
ampproject/owners-bot approvals met: false. ampproject/a4a, danielrozenberg, erwinmombay, rsimha, 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 torch2424 deleted the torch2424:amp-ima-video-hover-controls branch Jan 9, 2019

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.