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

Convert <video /> to amp-video #4

Closed
mjangda opened this issue Sep 29, 2015 · 6 comments
Closed

Convert <video /> to amp-video #4

mjangda opened this issue Sep 29, 2015 · 6 comments
Labels
Enhancement New feature or improvement of an existing one Integration: Jetpack

Comments

@mjangda
Copy link
Contributor

mjangda commented Sep 29, 2015

See https://github.com/ampproject/amphtml/blob/master/builtins/amp-video.md

WPCOM + Jetpack: Should handle VideoPress as well.

@mjangda mjangda added the Enhancement New feature or improvement of an existing one label Sep 29, 2015
@mjangda mjangda added this to the Developer Preview milestone Sep 29, 2015
@mjangda mjangda mentioned this issue Oct 28, 2015
@EnigmaSolved
Copy link

It looks like AMP-WP currently only handles VideoPress if it is used in the Iframe mode (which is the default), but it is possible to configure VideoPress to fully embed in the page (ie, no Iframe), which then allows for customizing the styling. This can be done via adding the following to the functions.php file:

// This forces VideoPress to embed into the page (to allow for styling, etc.), instead of using an Iframe (which is the default).
function my_videopress_use_iframe($vp_iframe){
    return false; // True indicates that Iframe will be used; False, that player will be fully embedded into page.
}
add_filter('jetpack_videopress_player_use_iframe', 'my_videopress_use_iframe', 1000);

I use the above (because I want to customize the CSS of the player), and so currently there is no Amp output for my VideoPress videos. I'm not sure what the best route here is--perhaps outputting the same <amp-iframe> VideoPress content in this situation as well?

Somehow we need to eventually handle this condition though, as one of the advertised features of VideoPress is customizing the CSS (though admittedly it's listed as being a Beta feature currently), and that can only be done via the above method (as far as I know).

Thoughts?

@mjangda
Copy link
Contributor Author

mjangda commented Feb 19, 2016

Yeah, falling back to amp-iframe would be the only option here since the AMP spec does not support Flash embeds.

@EnigmaSolved
Copy link

Actually I think the newest version of the VideoPress player no longer uses Flash (I believe it's a combination HTML5 Video and JS), though it is possible to force it to use the "legacy" Flash-based player. But it seems like it might be more trouble than it's worth to try to make things work in the fully-embedded-in-page mode (vs Iframe) for AMP pages as it needs to inject it's own videopress.js in order for the player to work, and that just seems like a nightmare to work with in the AMP context (the more I think about it). So yeah, falling back to the same amp-iframe content is probably the way to go.

@mjangda
Copy link
Contributor Author

mjangda commented Feb 20, 2016

Would you mind linking to an example post? I don't have VideoPress readily available on a non-WordPress.com site right now. The non-iframe mode should still work if it's outputting video tags, just without the customizations.

@EnigmaSolved
Copy link

I realize now why it's not working--both the legacy player and the new player (in non-iframe mode) build the player itself via Javascript after page load, so there's very little output prior to that (and thus very little available to the Amp-WP plugin to access). However, I do think that little bit (<div id="v-Gd7RSV6s-1" class="video-player"><div id="v-Gd7RSV6s"></div>) could be parsed into something useful.

I realize that it isn't much, but the Gd7RSV6s is the id of the video, which is the most important piece needed in order to construct the correct Iframe string. For example, currently the below is the Iframe embed string for the above (that is generated by the VideoPress Share menu):
<iframe width="560" height="315" src="https://videopress.com/embed/Gd7RSV6s" frameborder="0" allowfullscreen></iframe>
The only problem I see at the moment is that we won't have access (currently) to the width and height. Will amp-iframe allow for a iframe without defined dimensions, and furthermore I'm not sure whether we'd end up with funky result or not without dimensions anyway (maybe we could do width=100%, height=auto?)?

I don't want to link to my testing site (as it's private), but I have added some code to my live site so that you can toggle between the Legacy player and the New player (configured to fully embed in the page, and not use Iframes).

Legacy Player:
https://transitionsmft.org/blog/733/how-can-couples-prepare-for-future-conflicts/amp/?video_player=1
https://transitionsmft.org/blog/733/how-can-couples-prepare-for-future-conflicts/?video_player=1

New Player:
https://transitionsmft.org/blog/733/how-can-couples-prepare-for-future-conflicts/amp/?video_player=0
https://transitionsmft.org/blog/733/how-can-couples-prepare-for-future-conflicts/?video_player=0

If you view the non-Amp versions of this page the CSS is a bit buggered for the new player (particularly in the Share menu) because I've not updated the public site for that yet.

I should also mention that you may notice some VideoObject Schema data in the page, but that is not native to VideoPress. I basically built a wrapper for VideoPress which I use with a shortcode to output the correct Schema data for a given video (which also gives me the ability to define a description, which is not currently possible with VideoPress). So, none of that data is going to be available to other users for parsing by Amp-WP, unfortunately.

If we go this route (of building an Iframe string off of the video id, eg, https://videopress.com/embed/Gd7RSV6s) it might be good to reach out to the VideoPress devs to ask that they ping someone on the Amp-WP team if they ever change the embed string (since we'll be manually constructing it under the above plan, as opposed to just parsing it from what is outputted). Though, it does seem unlikely they'd change something like that anytime soon and without building in a server-side mechanism for handling the old links.

Alternatively (to the above plan), if the VideoPress devs can make it watch for when a page is in "Amp mode" and then actually output more content that would greatly simplify things! It seems all that would be needed is just to output the single Iframe string (not even bother with the videopress.js as it would be ignored anyway, I believe. That might actually be the best route, now that I think about it. VideoPress can just test for the Amp-WP function (can't remember the name at the moment) that indicates whether the page is an AMP page or not, and then test the result of the function if it exists, and if it's an AMP page just output the VideoPress Iframe tag (regardless of whether the user has set their VideoPress to not use Iframes).

:)

@EnigmaSolved
Copy link

Don't know why I didn't think of this before... This is the simplest solution for now (perhaps this can be documented somewhere for the sake of VideoPress users who are not using the default mode of Iframes?). In functions.php add the following:

function my_videopress_use_iframe($vp_iframe){
// True indicates that Iframe will be used; False, that player will be fully embeded into page (eg, for CSS styling, etc.).
    if( function_exists( 'is_amp_endpoint' ) && is_amp_endpoint() ){
        return true; // Return Iframe.
    }
    else{
        return false; // Fully embed player into page (ie, no Iframe used).
    }
}
add_filter('jetpack_videopress_player_use_iframe', 'my_videopress_use_iframe', 1000);

This checks if the Amp-WP plugin is active, and if so checks whether we are on an AMP page or not. And if we are, then the above tells VideoPress to output an Iframe (which can then be properly parsed by the Amp-WP plugin). Otherwise, forego the Iframe and fully embed the VideoPress player in the page (so that it can be styled, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one Integration: Jetpack
Projects
None yet
Development

No branches or pull requests

2 participants