AMP jwplayer #2660
AMP jwplayer #2660
Conversation
# Conflicts: # validator/validator.protoascii
|
👍 @pablos1 Ready for review? |
|
@rudygalfi Yep, review away |
|
|
||
| /** @override */ | ||
| layoutCallback() { | ||
| const width = this.element.getAttribute('width'); |
There was a problem hiding this comment.
Does jwplayer support poster images that can be shown without loading the player?
There was a problem hiding this comment.
@cramforce We do support poster images in the platform, but not all media assets have them.
There was a problem hiding this comment.
Check out how we do this for youtube. It has handling for the case where the poster image is missing.
| @@ -0,0 +1,115 @@ | |||
| <!--- | |||
| Copyright 2015 Longtail Ad Solutions Inc. | |||
There was a problem hiding this comment.
Are you guys ok to contributing the code to the AMP Project and switching the copyright to " 2016 The AMP HTML Authors. All Rights Reserved." ? The one currently in place won't pass the projects presubmit checks.
There was a problem hiding this comment.
Yes, @erwinmombay , fine with us to change the copyright to AMP.
| } | ||
|
|
||
| /** @override */ | ||
| documentInactiveCallback() { |
There was a problem hiding this comment.
This has been renamed #pauseCallback. It's the same, but you don't have to return any value.
|
@dvoytenko @rudygalfi All good? Anything else I can do here? |
|
I'll defer to @dvoytenko on if more changes are needed, but before final submission you'll need to squash commits. |
| }).catch(() => { | ||
| // Thumbnails aren't available for all media content. | ||
| // On a 404, remove the placeholder image. | ||
| this.element.removeChild(imgPlaceholder); |
There was a problem hiding this comment.
Please do this part as:
}).catch(() => {
this.deferMutate(() => {
this.element.removeChild(imgPlaceholder);
});
});
Or alternatively, you can wait for image to load before adding it to DOM. That way you can avoid playing with visibility as well.
|
@pablos1 Looks great. Just a couple of comments. Could you please also squash commits? |
|
@dvoytenko Accidentally deleted the forked branch during the squash. It's back with the suggested changes. Could you re-open the PR? |
|
I'm unable to re-open. You might need to start a new one. |
|
I was afraid of that. #2709. |
Implements #2125