Skip to content

amp-youtube: Autoplay animation to pause when video ends#9602

Merged
aghassemi merged 1 commit intoampproject:masterfrom
wassgha:bugfix-youtube-stop
May 31, 2017
Merged

amp-youtube: Autoplay animation to pause when video ends#9602
aghassemi merged 1 commit intoampproject:masterfrom
wassgha:bugfix-youtube-stop

Conversation

@wassgha
Copy link
Copy Markdown
Contributor

@wassgha wassgha commented May 27, 2017

This fixes the bug where Youtube videos do not send a pause event when ended ( https://github.com/ampproject/amphtml/issues/5776 ).

Changes

  • Changed example to use shorter video to see effect
  • Changed initial state of videos to -1 (UNSTARTED) instead of 0 (ENDED) based on the Youtube API
  • Added "ENDED" state (state 0)
  • Added unit test for this behavior

Closes #5776

@googlebot
Copy link
Copy Markdown

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@wassgha wassgha force-pushed the bugfix-youtube-stop branch 3 times, most recently from fe99823 to b2a5614 Compare May 27, 2017 05:59
@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

Copy link
Copy Markdown
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

Awesome!

For the PR title, let's do:
"amp-youtube: autoplay animation to pause when video ends"

PR titles end up being the commit message which show up in Release Notes. Best titles would be something that will be useful to an AMP user reading the release notes.

<amp-youtube
id="myVideo"
data-videoid="lBTCB7yLs8Y"
data-videoid="a2bESrN3Hig"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's change back to lBTCB7yLs8Y
a2bESrN3Hig almost gave me a headache :)

* @private
*/

/* Correct PlayerStates taken from: https://developers.google.com/youtube/iframe_api_reference#Playback_status */
Copy link
Copy Markdown
Contributor

@aghassemi aghassemi May 30, 2017

Choose a reason for hiding this comment

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

// for non-method signature comments

@wassgha wassgha force-pushed the bugfix-youtube-stop branch from b2a5614 to ba5db0d Compare May 30, 2017 22:49
@wassgha wassgha force-pushed the bugfix-youtube-stop branch from ba5db0d to 43889d6 Compare May 30, 2017 22:54
@wassgha wassgha changed the title Fixed bug #5776 and added unit tests amp-youtube: Autoplay animation to pause when video ends May 30, 2017
@wassgha
Copy link
Copy Markdown
Contributor Author

wassgha commented May 30, 2017

Made the changes!

Copy link
Copy Markdown
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

LGTM. will merge when CI passes.

@aghassemi aghassemi merged commit d432b21 into ampproject:master May 31, 2017
@wassgha wassgha deleted the bugfix-youtube-stop branch May 31, 2017 21:56
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.

YouTube does not dispatch a pause event after video ends (unlike HTML5 video)

4 participants