🐛amp-ima-video: Allowed Replaying / Looping video on amp-ima-video#18695
🐛amp-ima-video: Allowed Replaying / Looping video on amp-ima-video#18695torch2424 merged 9 commits intoampproject:masterfrom
Conversation
…p-ima-video-loop-18532
alanorozco
left a comment
There was a problem hiding this comment.
Ack.
This goes a little deep so it might take me a couple days to get to.
|
@alanorozco No worries, whenever you get the time 😄 As long as you don't mind the occasional ping haha! |
ads/google/imaVideo.js
Outdated
| */ | ||
| export function getPropertiesForTesting() { | ||
| return {adContainerDiv, adRequestFailed, adsActive, adsManagerWidthOnLoad, | ||
| return {adContainerDiv, allAdsCompleted, |
There was a problem hiding this comment.
Oof :)
Not your fault, but would you mind setting one item per line?
ads/google/imaVideo.js
Outdated
| bigPlayDiv.addEventListener('tapwithoutdrag', onClick.bind(null, global)); | ||
| bigPlayDiv.addEventListener( | ||
| 'tapwithoutdrag', | ||
| onBigPlayClick.bind(null, global) |
There was a problem hiding this comment.
Use arrow funcs here instead of bind.
Might wanna consider back-changing the other events but it's fine to leave as-is.
There was a problem hiding this comment.
Personally, I think it'd be more beneficial to have it stay consistent. Helps maintainability in my opinion for someone jumping in, rather than trying to understand why this one is different. Especially since there are so many events already.
I definitely agree they should all be arrow functions, but I think that should be done in a larger refactor.
What do you think? 😄
ads/google/imaVideo.js
Outdated
| bigPlayDiv.addEventListener( | ||
| interactEvent, | ||
| onBigPlayClick.bind(null, global) | ||
| ); |
There was a problem hiding this comment.
Collapse closing paren into previous line.
ads/google/imaVideo.js
Outdated
| // If all ads are not completed, | ||
| // onContentResume will show the bigPlayDiv | ||
| if (allAdsCompleted) { | ||
| setStyle(bigPlayDiv, 'display', 'table-cell'); |
There was a problem hiding this comment.
Trying to stay consistent with other control element styling, and don't want to cause any weird CSS bugs by changing the way the elements are being laid out.
Definitely agree the styling on the controls should be improved though. Hopefully in a later refactor if I get an "Okay" on that.
What do you think? 😄
There was a problem hiding this comment.
SGTM, but, if possible, try to keep this value in a single source. Either by reading the calculated style or by setting it some other way by default.
|
@alanorozco This should be good for another review 😄 Please see my comments, one of them is hidden because of the |
ads/google/imaVideo.js
Outdated
| // If all ads are not completed, | ||
| // onContentResume will show the bigPlayDiv | ||
| if (allAdsCompleted) { | ||
| setStyle(bigPlayDiv, 'display', 'table-cell'); |
There was a problem hiding this comment.
SGTM, but, if possible, try to keep this value in a single source. Either by reading the calculated style or by setting it some other way by default.
ads/google/imaVideo.js
Outdated
| playbackStarted, | ||
| playerState, | ||
| PlayerStates, | ||
| bigPlayDiv, |
There was a problem hiding this comment.
I realized this is alphabetically sorted but bigPlayDiv is in the wrong order :)
| playbackStarted = true; | ||
| uiTicker = setInterval(uiTickerClick, 500); | ||
| setInterval(playerDataTick, 1000); | ||
| bigPlayDiv.removeEventListener(interactEvent, onClick); |
There was a problem hiding this comment.
One question: do we need to removeEventListener to interactEvent, and add it back when allAdsCompleted or onContentResume? Personally I don't know how much resource can it save give we've already set display to none. Up to you.
There was a problem hiding this comment.
Ahhh so we don't want to remove the event listener here, because we may need it in the future. Since the big play div is hidden, the event listener wont be fired, and unlike the controls, it doesn't conflict with another event listener on the video itself 😄
There was a problem hiding this comment.
Discussed offline. Will remove/add as needed
There was a problem hiding this comment.
Discussed more offline, this falls more under a seperate refactoring PR. Since previously there was no method of adding/removing dynamically and all that.
…mpproject#18695) * Added a replay icon from google material icons * LEft a TODO * Need to dtermine when post-roll to show big play again * Finished tests for showing the big play div * Fixed all commit checks * Hopefully fixed flaky tests with single video mock * Made PR comments * Made remaining PR Changes
closes #18532
Following the design of , this now shows the "Big Play" button on end of video, with consideration of post-roll ads, and allows replaying / looping of a video.
Also, made some updates to the
ima-videoexample 😄Example