Add ability to go canonical AMP (AMP-only) #425

pbakaus · 2016-07-29T03:47:36Z

This directly addresses #394 and adds the following:

Configuration page under Settings with one setting (Canonical AMP), off by default right now
A WP option called amp_canonical that the plugin, plus any other plugins and themes can use to do certain things differently when signalled the blog is in "full AMP mode".
A theme support flag called amp that the theme can use to signal it supports AMP.
Renders the post content of all posts (only single posts right now) as AMP and does a few tricks to make WP behave nicely.

Importantly, this PR does not address the actual used theme – it's still the theme's responsibility right now to make sure no external JS or CSS is loaded.

Where can I see a demo?

Glad you're asking. A live example of a canonical AMP blog + theme using this plugin can be found here: https://paulbakaus.com

How do I make my theme validate?

Here's how a theme would conform:

1. Add `amp` theme support

The theme must signal that it supports AMP in order for the plugin to enable canonical AMP mode, like so:

add_theme_support( 'amp' );

2. Inline stylesheet

Something like:

function add_inline_stylesheet() {
    if( get_option( 'amp_canonical') ) {
        $css = file_get_contents(get_stylesheet_uri());
        ?>
        <style amp-custom><?= $css; ?></style>
        <?php
    }
}

add_action( 'wp_head', 'add_inline_stylesheet' );

3. Provide valid header bits

Include the flash in the html tag (doesn't hurt even without AMP), make sure that the charset is lowercase and that the correct viewport is set.

<html ⚡ <?php language_attributes(); ?>>
    <head>
        <meta charset="<?= strtolower(get_bloginfo( 'charset', 'display' )); ?>">
        <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">

4. Embed AMP analytics

Use AMP analytics instead of other analytics solutions based on the AMP_CANONICAL check:

if ( get_option( 'amp_canonical') ) {
?>
<amp-analytics type="googleanalytics" id="analytics-ga">
  <script type="application/json">
  {
    "vars": {
      "account": "UA-2427667-1"
    },
    "triggers": {
      "trackPageview": {
        "on": "visible",
        "request": "pageview"
      }
    }
  }
  </script>
</amp-analytics>
<?php
}

5. Optional: Add AMP-specific styles

Currently, stolen from the styles in the default template. These are not required but helpful:

.amp-wp-enforced-sizes {
    /** Our sizes fallback is 100vw, and we have a padding on the container; the max-width here prevents the element from overflowing. **/
    max-width: 100%;
}

.amp-wp-unknown-size img {
    /** Worst case scenario when we can't figure out dimensions for an image. **/
    /** Force the image into a box of fixed dimensions and use object-fit to scale. **/
    object-fit: contain;
}

amp-carousel > amp-img > img {
    object-fit: contain;
}

This is my first real attempt at writing Wordpress-compliant code and I haven't touched PHP for ages, so a lot of it is probably not compliant or can be done in better ways. Very much looking for feedback and somebody who can take it and polish it if needed.

mjangda · 2016-08-03T20:48:04Z

Thanks for tackling this @pbakaus! Looking pretty good at a quick glance. I'll take it for a more thorough test drive and code review next week.

…port check

pbakaus · 2016-08-03T21:59:12Z

Sounds good @mjangda! I just followed up with a few fixes, mostly to make it play nicer in the WP world by using get_option instead of a constant, and adding a theme_support flag so themes can signal they're AMP ready. This gives us an easy way forward to show valid AMP themes.

pbakaus · 2016-08-22T17:06:51Z

friendly ping!

mjangda · 2016-08-23T06:29:47Z

Considering this is still pretty experimental, I wonder if we should remove the options page + toggle for now and let power-users enable via the add_theme_support call.

Mostly trying to avoid a barrage of support requests complaining about all the indexing errors they're getting from GWT (we get enough of those as it is :))

pbakaus · 2016-08-23T17:28:31Z

:) We can, but does that work for WP-hosted blogs as well (I have never modified a theme in a hosted WP blog). If so, I'm cool with it.

pbakaus · 2016-08-23T17:40:32Z

Oh also, I'm looking for advice on how to properly add canonical AMP support for non-single pages (e.g. search, homepage, archive etc.). Any thoughts?

pbakaus · 2016-08-27T17:58:39Z

@mjangda let me know which way to proceed.

mjangda · 2016-08-30T04:32:10Z

includes/amp-canonical-filters.php

@@ -0,0 +1,19 @@
+<?php
+
+// before Jetpack ever gets loaded, we need to remove a link rel prefetch for canonical AMP support


Jetpack adds a bunch of dns-prefetch tags (not just for VideoPress) so a better approach might be to unhook it completely:

function amp_canonical_disable_jetpack_dns_fetch() { if ( class_exists( 'Jetpack' ) ) { remove_action( 'wp_head', array( 'Jetpack', 'dns_prefetch' ) ); } } add_action( 'wp_head', 'amp_canonical_disable_jetpack_dns_fetch', 0 ); // Hook early so we can unhook Jetpack

Excellent, I've used your code instead.

jdevalk · 2016-09-23T09:15:15Z

Seems to me this needs #188 to be fully useful. Anything we from Yoast can do to help speed this whole thing up? It's going very slowly whereas AMP development in general seems to be nice and fast, and we'd love to make this all work.

moorscode · 2016-09-23T09:24:55Z

includes/amp-canonical-actions.php

+}
+
+// Generate the AMP post content early on (runs the_content filters but skips our filter below)
+$GLOBALS['amp_content'] = build_post_content();


I would advice strongly against introducing any $GLOBALS.

I haven't touched PHP in a while – can you point me to a better way of doing this?

… correct check, rename a function to a sane name

pbakaus · 2016-09-24T00:56:11Z

@mjangda please take another look.

I've fixed a bunch of issues, and have reworded the provided option, clearly stating it's experimental, and linking out to the amended readme to explain exactly what it does, and doesn't.

I do think it's important to be able to toggle it on/off, even in hosted environments, and the theme having support for AMP should not be the signal for "I want it all turned into AMP". Themes that support AMP should allow for both render mods – non-AMP and AMP, and it should be up to the user to decide.

What are your remaining blockers to get this merged and deployed as new version?

pbakaus · 2016-09-24T01:00:56Z

@jdevalk while I think #188 would be beneficial, I don't think it should be a blocking requirement.

This PR is useful on its own, and will turn all posts into valid AMP when done correctly (like on my blog), while rendering everything else normally – the theme still looks the same, the transition for the user is seamless.

mjangda

Thanks again for working through this, Paul.

Left a bunch of feedback throughout the PR. Please let me know if anything is not clear. It would also be good to ensure new code is following WordPress Coding Standards especially around spacing and indentation.

mjangda · 2017-02-13T05:11:31Z

options.php

+function amp_register_settings() {
+     register_setting(
+          'amp_options',  // settings section
+          'amp_canonical' // setting name


We should pass in a third arg here, which at a minimum, specifies the type, sanitize_callback, and default. (see https://developer.wordpress.org/reference/functions/register_setting/#parameters)

mjangda · 2017-02-13T05:13:52Z

amp.php

-	add_rewrite_endpoint( AMP_QUERY_VAR, EP_PERMALINK );
-	add_post_type_support( 'post', AMP_QUERY_VAR );
+	// Add rewrite endpoints if we either don't want canonical AMP, or if the theme doesn't support it
+	if( ! get_option( 'amp_canonical') || ! get_theme_support('amp') ) {


We should probably have a helper function to check if canonical mode is enabled (and then use it wherever applicable).

mjangda · 2017-02-13T05:15:55Z

amp.php

+
+// load high priority filters for canonical AMP
+if( get_option( 'amp_canonical') ) {
+	require_once( AMP__DIR__ . '/includes/amp-canonical-filters.php' );


We can move this to amp_render_canonical.

mjangda · 2017-02-13T05:20:51Z

options.php

+<?php
+
+add_action( 'admin_init', 'amp_register_settings' );
+add_action( 'admin_menu', 'amp_custom_admin_menu' );


We need an easy way to disable this the options page in certain contexts (e.g. across all of WordPress.com). To do so, we could:

Rename amp_register_settings to amp_admin_init

Move the admin_menu hook into amp_admin_init

Add a filter in amp_admin_init that allows skipping registration of the admin pages.

mjangda · 2017-02-13T05:21:58Z

includes/amp-canonical-actions.php

+	// Always include AMP form & analytics, as almost every template has search form somewhere and is tracking page views
+	$scripts = array_merge(
+		array('amp-form' => 'https://cdn.ampproject.org/v0/amp-form-0.1.js'),
+		array('amp-analytics' => 'https://cdn.ampproject.org/v0/amp-analytics-0.1.js'),


Should we leave this to themes/plugins to manage instead of assuming that all pages will have a form and/or analytics?

mjangda · 2017-02-13T05:24:47Z

includes/amp-helper-functions.php

@@ -18,7 +18,7 @@ function amp_get_permalink( $post_id ) {

 function post_supports_amp( $post ) {
 	// Because `add_rewrite_endpoint` doesn't let us target specific post_types :(
-	if ( ! post_type_supports( $post->post_type, AMP_QUERY_VAR ) ) {
+	if (! get_option( 'amp_canonical') && ! post_type_supports( $post->post_type, AMP_QUERY_VAR ) ) {


Should we always return true when in canonical mode?

mjangda · 2017-02-13T05:27:16Z

amp.php

@@ -116,6 +122,15 @@ function amp_render() {
 	exit;
 }

+function amp_render_canonical() {


We should rename this to amp_add_canonical_actions for clarity.

mjangda · 2017-03-25T21:36:10Z

Continued in #668

pbakaus added 2 commits July 28, 2016 20:25

Added configuration option to turn whole site into AMP (self canonical)

103b70c

removed some accidental leftovers

7c1e714

pbakaus mentioned this pull request Jul 29, 2016

Support for canonical AMP (AMP only) #394

Closed

pbakaus added 3 commits July 29, 2016 22:39

more sensible max width for canonical AMP (to support Desktop)

c9a44ff

don't run the_content filters twice

f3aede5

added a missing include

f2e1a95

switched from using a constant to WP style option and added theme sup…

7469e54

…port check

fix merge

f019bd7

mjangda reviewed Aug 30, 2016
View reviewed changes

moorscode reviewed Sep 23, 2016

View reviewed changes

pbakaus added 2 commits September 23, 2016 18:49

disable all prefetch for jetpack, replace AMP_CANONICAL instance with…

10a4c93

… correct check, rename a function to a sane name

better description for the option, readme for the option

b719da1

mjangda suggested changes Feb 13, 2017

View reviewed changes

amedina mentioned this pull request Mar 23, 2017

Add capability to generate AMP-only content (AMP-as-Canonical) #668

Closed

mjangda closed this Mar 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to go canonical AMP (AMP-only) #425

Add ability to go canonical AMP (AMP-only) #425

pbakaus commented Jul 29, 2016 •

edited

Loading

mjangda commented Aug 3, 2016

pbakaus commented Aug 3, 2016

pbakaus commented Aug 22, 2016

mjangda commented Aug 23, 2016

pbakaus commented Aug 23, 2016

pbakaus commented Aug 23, 2016

pbakaus commented Aug 27, 2016

mjangda Aug 30, 2016

pbakaus Sep 24, 2016

jdevalk commented Sep 23, 2016

moorscode Sep 23, 2016

pbakaus Sep 23, 2016

pbakaus commented Sep 24, 2016

pbakaus commented Sep 24, 2016

mjangda left a comment

mjangda Feb 13, 2017

mjangda Feb 13, 2017

mjangda Feb 13, 2017

mjangda Feb 13, 2017

mjangda Feb 13, 2017

mjangda Feb 13, 2017

mjangda Feb 13, 2017

mjangda commented Mar 25, 2017

		@@ -0,0 +1,19 @@
		<?php

		// before Jetpack ever gets loaded, we need to remove a link rel prefetch for canonical AMP support

Add ability to go canonical AMP (AMP-only) #425

Add ability to go canonical AMP (AMP-only) #425

Conversation

pbakaus commented Jul 29, 2016 • edited Loading

Where can I see a demo?

How do I make my theme validate?

1. Add amp theme support

2. Inline stylesheet

3. Provide valid header bits

4. Embed AMP analytics

5. Optional: Add AMP-specific styles

mjangda commented Aug 3, 2016

pbakaus commented Aug 3, 2016

pbakaus commented Aug 22, 2016

mjangda commented Aug 23, 2016

pbakaus commented Aug 23, 2016

pbakaus commented Aug 23, 2016

pbakaus commented Aug 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdevalk commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbakaus commented Sep 24, 2016

pbakaus commented Sep 24, 2016

mjangda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjangda commented Mar 25, 2017

pbakaus commented Jul 29, 2016 •

edited

Loading

1. Add `amp` theme support