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: Allowed Replaying / Looping video on amp-ima-video #18695

Merged
merged 9 commits into from Oct 18, 2018

Conversation

torch2424
Copy link
Contributor

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-video example 馃槃

Example

loopimavideo

Copy link
Member

@alanorozco alanorozco left a comment

Choose a reason for hiding this comment

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

Ack.

This goes a little deep so it might take me a couple days to get to.

@torch2424
Copy link
Contributor Author

@alanorozco No worries, whenever you get the time 馃槃

As long as you don't mind the occasional ping haha!

@@ -1244,10 +1282,11 @@ function postMessage(data) {
* @visibleForTesting
*/
export function getPropertiesForTesting() {
return {adContainerDiv, adRequestFailed, adsActive, adsManagerWidthOnLoad,
return {adContainerDiv, allAdsCompleted,
Copy link
Member

Choose a reason for hiding this comment

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

Oof :)

Not your fault, but would you mind setting one item per line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely 馃槃

bigPlayDiv.addEventListener('tapwithoutdrag', onClick.bind(null, global));
bigPlayDiv.addEventListener(
'tapwithoutdrag',
onBigPlayClick.bind(null, global)
Copy link
Member

Choose a reason for hiding this comment

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

Use arrow funcs here instead of bind.

Might wanna consider back-changing the other events but it's fine to leave as-is.

Copy link
Contributor Author

@torch2424 torch2424 Oct 18, 2018

Choose a reason for hiding this comment

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

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? 馃槃

Copy link
Member

Choose a reason for hiding this comment

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

sg.

bigPlayDiv.addEventListener(
interactEvent,
onBigPlayClick.bind(null, global)
);
Copy link
Member

Choose a reason for hiding this comment

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

Collapse closing paren into previous line.

// If all ads are not completed,
// onContentResume will show the bigPlayDiv
if (allAdsCompleted) {
setStyle(bigPlayDiv, 'display', 'table-cell');
Copy link
Member

Choose a reason for hiding this comment

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

Why table-cell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? 馃槃

Copy link
Member

Choose a reason for hiding this comment

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

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.

@torch2424
Copy link
Contributor Author

@alanorozco This should be good for another review 馃槃 Please see my comments, one of them is hidden because of the outdated tag.

// If all ads are not completed,
// onContentResume will show the bigPlayDiv
if (allAdsCompleted) {
setStyle(bigPlayDiv, 'display', 'table-cell');
Copy link
Member

Choose a reason for hiding this comment

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

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.

playbackStarted,
playerState,
PlayerStates,
bigPlayDiv,
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 馃槃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. Will remove/add as needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed more offline, this falls more under a seperate refactoring PR. Since previously there was no method of adding/removing dynamically and all that.

@torch2424 torch2424 merged commit ef86e63 into ampproject:master Oct 18, 2018
@torch2424 torch2424 deleted the amp-ima-video-loop-18532 branch October 19, 2018 00:00
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
鈥pproject#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

amp-ima-video: Unable to loop / restart a video
4 participants