Implementation of VideoInterface for amp-gfycat player #10583
Conversation
| @@ -36,6 +36,10 @@ tags: { # <amp-gfycat> | |||
| mandatory: true | |||
| } | |||
| attrs: { | |||
| name: "autoplay" | |||
There was a problem hiding this comment.
minor nit: we like to keep attributes in alphabetical order. if not too much trouble, move above data-gfyid
There was a problem hiding this comment.
@honeybadgerdontcare Done
|
@aghassemi validation looks good. |
|
|
||
| /** @override */ | ||
| isInteractive() { | ||
| return true; |
There was a problem hiding this comment.
set this to false so you don't get the sound equalizer icon and the autoplay shim which are not needed in this component.
| this.videoIframeSrc_ = null; | ||
|
|
||
| /** @private {?Promise} */ | ||
| this.playerReadyPromise_ = null; |
There was a problem hiding this comment.
this and playerReadyResolver_ should not be needed. See below for alternative.
| buildCallback() { | ||
| this.videoid_ = this.getVideoId_(); | ||
|
|
||
| this.playerReadyPromise_ = new Promise(resolve => { |
| createPlaceholderCallback() { | ||
| const placeholder = this.win.document.createElement('amp-img'); | ||
| dev().assert(this.videoid_); | ||
| const videoid = this.videoid_ || ''; |
There was a problem hiding this comment.
no need to do || '' anymore since dev.assert() above ensures videoid_ is not falsy.
| return this.videoIframeSrc_; | ||
| } | ||
|
|
||
| const videoid = this.videoid_ || ''; |
There was a problem hiding this comment.
just dev().assert(this.videoid_);, videoid_ should not be null by the time we get here.
examples/gfycat.amp.html
Outdated
| width="640" | ||
| height="360" | ||
| data-autoplay="0"> | ||
| autoplay> |
There was a problem hiding this comment.
we can't have both noautoplay and autoplay see below for a solution.
| * @private | ||
| * */ | ||
| sendCommand_(command, opt_arg) { | ||
| this.playerReadyPromise_.then(() => { |
There was a problem hiding this comment.
in your case, there is no need to wait for player ready promise
| } | ||
|
|
||
| if (eventData == 'ready') { | ||
| this.element.dispatchCustomEvent(VideoEvents.LOAD); |
There was a problem hiding this comment.
hmm, iframe load was also dispatching VideoEvents.LOAD only one is needed. For some players, iframe being loaded is still too early to be considered "ready" that's why they have this custom message. If that the case for you as well? Does this event come from the player at all? Pick between this or iframe load, we only need one.
extensions/amp-gfycat/amp-gfycat.md
Outdated
| @@ -83,6 +83,26 @@ Example: Finding the actual width and height | |||
| </iframe> | |||
| ``` | |||
|
|
|||
| ##### autoplay | |||
There was a problem hiding this comment.
as discussed above, we can't do both autoplay and noautoplay
| @@ -32,6 +32,10 @@ tags: { # <amp-gfycat> | |||
| disallowed_ancestor: "AMP-SIDEBAR" | |||
| requires_extension: "amp-gfycat" | |||
| attrs: { | |||
| name: "autoplay" | |||
There was a problem hiding this comment.
remove as discussed above.
|
validation changes still look good. |
|
@aghassemi I made all the changes you requested. Can you please review? |
| /** @override */ | ||
| createPlaceholderCallback() { | ||
| const placeholder = this.win.document.createElement('amp-img'); | ||
| dev().assert(this.videoid_); |
There was a problem hiding this comment.
for these two lines, pleas do:
const videoId = dev().assertString(this.videoid_);
| return this.videoIframeSrc_; | ||
| } | ||
|
|
||
| const videoid = this.videoid_; |
There was a problem hiding this comment.
ditto: const videoId = dev().assertString(this.videoid_);
| if (noautoplay) { | ||
| src += '?autoplay=0'; | ||
| params['autoplay'] = '0'; |
There was a problem hiding this comment.
hmm, with this PR, AMP completely manages autoplay, I believe you want to set autoplay=0 always now, no?
There was a problem hiding this comment.
@aghassemi I fixed video id assert.
About autoplay=0 - yes, AMP manages video autoplay. But an iframe needs this attribute to be 0 only when iframe doesn't autoplay, it uses it for initialization, to set initial state for controls.
There was a problem hiding this comment.
Also the second build - integration_tests - keeps failing, I'm not sure why. I'm keeping this branch up to date with the latest master.
There was a problem hiding this comment.
got it, might be worth documenting this reason as code comment
I restarted integration tests, looks like a flake unrelated to your changes.
| if (noautoplay) { | ||
| src += '?autoplay=0'; | ||
| params['autoplay'] = '0'; |
There was a problem hiding this comment.
got it, might be worth documenting this reason as code comment
I restarted integration tests, looks like a flake unrelated to your changes.
|
@aghassemi cool, thanks. So this change will be in production next week? |
|
@koshka, yes, next Wednesday. Thanks implementing the video-interface! |
|
The validator changes are now live in prod. |
Implementation of VideoInterface for amp-gfycat player
Opened issue for this PR #10537