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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow prerendering amp-video poster #1718

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

mkhatib
Copy link
Contributor

@mkhatib mkhatib commented Feb 1, 2016

Not entirely sure this is the way to go about this, but happy to take another stab at this

Closes #1657

const posterAttr = this.element.getAttribute('poster');

if (!posterAttr) {
console/*OK*/.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to spam readers with warnings? Should it be protected by devmode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Will add development condition.

Copy link
Member

Choose a reason for hiding this comment

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

If we protect it by devmode, lets make it an error :)

@mkhatib
Copy link
Contributor Author

mkhatib commented Feb 9, 2016

Ok, as discussed face to face, we'll be using build callback to create and append the required dom for prerendering. For example, in the case of amp-video poster, we'll move the dom creation to buildCallback and set the preload=none and poster=<passed value>. On layoutCallback, we'll add the src of the video and set the preload the the passed value.

In a similar manner we're going to address the amp-youtube poster in prerender mode. Where we will automatically create an <img placeholder src="calculated-youtube-thumb-src"> in the buildCallback.

@dvoytenko let me know if I missed anything. I'll get the change ready and update this PR.

cc @jridgewell @cramforce

@dvoytenko
Copy link
Contributor

@mkhatib sg

@@ -16,8 +16,6 @@ limitations under the License.

### <a name="amp-facebook"></a> `amp-facebook`

**Status: Component landed, but does not validate.**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cramforce This is no longer the case right?

@mkhatib
Copy link
Contributor Author

mkhatib commented Feb 10, 2016

@dvoytenko PTAL, I am not sure this is the best way to do the tests, so if there's a better way happy to use that.

@@ -36,11 +36,31 @@ export function installVideo(win) {
}

/** @override */
layoutCallback() {
buildCallback() {
/** @private @const {?HTMLVideoElement} */
Copy link
Member

Choose a reason for hiding this comment

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

qq, how is this nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it should be, I just kept the same annotation type previously used, will update this to be not nullable (that's {!...} right?)

Copy link
Member

Choose a reason for hiding this comment

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

yep thats right 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. It should be non-nullable with !.

@mkhatib
Copy link
Contributor Author

mkhatib commented Feb 10, 2016

@dvoytenko thanks for all the comments 😄 PTAL

return loadPromise(video).then(() => {
setStyles(video, {visibility: ''});
return loadPromise(this.video_).then(() => {
setStyles(this.video_, {visibility: ''});
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove as well since we are not hiding it above. Basically we don't need then here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@dvoytenko
Copy link
Contributor

I think we are all good. Just one more comment.

@mkhatib
Copy link
Contributor Author

mkhatib commented Feb 10, 2016

Done. Thanks again 👍

@dvoytenko
Copy link
Contributor

LGTM. Please fix linter errors.

@mkhatib
Copy link
Contributor Author

mkhatib commented Feb 11, 2016

Added a small change to fix the flakey test that we talked about and rebased with master. Will merge this after tests run.

mkhatib added a commit that referenced this pull request Feb 11, 2016
Allow prerendering amp-video poster
@mkhatib mkhatib merged commit ae99b63 into ampproject:master Feb 11, 2016
@mkhatib mkhatib deleted the amp-video-poster branch February 11, 2016 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants