✨amp-ima-video: Show Controls on Hover#20130
Conversation
zhouyx
left a comment
There was a problem hiding this comment.
Thanks for the new feature! Looks cool. LGTM with one nit and one question.
ads/google/imaVideo.js
Outdated
| // Something blocked ima3.js from loading - ignore all IMA stuff and just play | ||
| // content. | ||
| videoPlayer.addEventListener(interactEvent, showControls); | ||
| addHoverEventToElement(/** @type !Element*/(videoPlayer), showControls); |
| } | ||
|
|
||
| // Hide controls after 3 seconds | ||
| if (playerState == PlayerStates.PLAYING) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah the mousemove event does fire a lot, but I was thinking about it and I essentially came to two conclusions:
- The event will only be fired on desktop devices, during certain sections of watching the video.
- 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 👍
There was a problem hiding this comment.
Plus one for throttling - if possible - though controlsVisible is clear and succinct!
| imaVideoObj.showControls | ||
| ); | ||
| videoPlayerElement.dispatchEvent(clickEvent); | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| // Hide controls after 3 seconds | ||
| if (playerState == PlayerStates.PLAYING) { |
There was a problem hiding this comment.
Plus one for throttling - if possible - though controlsVisible is clear and succinct!
ads/google/imaVideo.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Removess the appropriate event listener from an element |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ads/google/imaVideo.js
Outdated
|
|
||
| /** | ||
| * Adds the appropriate event listener to an element | ||
| * to represent a hover state. And adds an appropriate |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@zhouyx @curseagain made all requested changes! Added a throttler as well 😄 |
| * since this is applied per element, | ||
| * and would require wrapping the callback. | ||
| * Thus, the callback passed should be, | ||
| * appropriately throttled. See showControlsThrottled. |
|
@zhouyx This is good to go! Fixed the rate-limit build system thing 😄 |
zhouyx
left a comment
There was a problem hiding this comment.
Sorry for the late response here. LGTM. Thank you for the feature!!!
|
@zhouyx No worries! Thank you! |
* Need to fix check-types and make PR * Fixed check types * Fixed most pr comments * FInished all changes * Fixed the rate limit ads thing
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-videoto match the behavior ofamp-videoas much as possible, and this PR falls inline with that 😄.cc @curseagain
Example
Desktop
Mobile