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.

Sign up for GitHub

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

Jump to bottom

Allow prerendering amp-video poster #1718

Merged
merged 1 commit into from
Feb 11, 2016
Merged

Allow prerendering amp-video poster #1718

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

@mkhatib mkhatib mentioned this pull request Feb 1, 2016
Closed
dvoytenko
dvoytenko reviewed Feb 1, 2016
View reviewed changes
builtins/amp-video.js
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

mkhatib
mkhatib reviewed Feb 10, 2016
View reviewed changes
extensions/amp-facebook/amp-facebook.md
@@ -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.

erwinmombay
erwinmombay reviewed Feb 10, 2016
View reviewed changes
builtins/amp-video.js
@@ -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

dvoytenko
dvoytenko reviewed Feb 10, 2016
View reviewed changes
builtins/amp-video.js
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
@mkhatib
Merge pull request #1718 from mkhatib/amp-video-poster
ae99b63 
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
@gmajoulet gmajoulet mentioned this pull request Nov 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dvoytenko dvoytenko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mkhatib @dvoytenko @cramforce @erwinmombay