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

Add AMP theme support #1286

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@westonruter
Copy link

westonruter commented May 1, 2018

This is an exploration into adding amp theme support to Underscores. This new theme support flag is being introduced in v0.7 of the AMP plugin.

  • Splits up script and style enqueueing since the former can be isolated to be prevented in AMP.
  • Adds _s_is_amp() conditional function to check whether or not the response is being served as AMP .
  • Prevents enqueueing navigation.js. Uses the :focus-within peudo class instead of the focus class which was added via JS. This pseudo-class is supported by all browsers other than IE/Edge, and it ensures the submenu items are accessible even when JS is disabled. Also, amp-bind is used to manage the toggled state of the navigation menu on mobile. To get amp-bind to work, the toggle button has to be switched over to a button-like label which is connected to a hidden checkbox.
  • Prevent enqueueing skip-link-focus-fix.js since it is not allowed in AMP and it is only needed by IE11 anyway.
  • Adds support for using amp-live-list for the comment list, allowing new comments to be added to the page without reloading. A refresh time of 60 seconds is used by default. A _s_using_amp_live_list_comments() conditional is added to check whether amp-live-list should be used in the template.
  • The meta[viewport] is updated to add the AMP-required minimum-scale=1 value.

With these changes applied to a theme, a vanilla WordPress install with the AMP plugin active can generate responses that do not trigger any AMP validation errors.

For more details on changes coming in AMP v0.7, see the RC1 release post. See also another full example theme which implements amp theme support, and a WordPress install that has the theme active.

@westonruter westonruter force-pushed the xwp:amp-support branch from 90f673d to cdf8e2d May 1, 2018

@@ -24,6 +24,10 @@ function _s_setup() {
*/
load_theme_textdomain( '_s', get_template_directory() . '/languages' );
add_theme_support( 'amp', array(
'comments_live_list' => true,
) );

This comment has been minimized.

@westonruter

westonruter May 1, 2018

Naturally this shouldn't be merged as-is into master. It would need to be opt-in by the theme developer.

@westonruter

This comment has been minimized.

Copy link

westonruter commented May 1, 2018

I manually applied the changes to style.css as opposed to using sass to do so, since when I tried to compile it there were many differences. I suppose this is what #1159 is for.

/**
* Enqueue scripts.
*
* This short-circuits in AMP because custom scripts are not allowed. There is are AMP equivalents provided elsewhere.

This comment has been minimized.

@westonruter
header.php Outdated
[aria-expanded]="siteNavigationMenu.expanded ? 'true' : 'false'"
>
<?php esc_html_e( 'Primary Menu', '_s' ); ?>
</label>

This comment has been minimized.

@westonruter

westonruter May 3, 2018

Should be indented.

header.php Outdated
type="checkbox"
hidden
on="change:AMP.setState( { siteNavigationMenu: { expanded: event.checked } } )"
>

This comment has been minimized.

@westonruter

westonruter May 3, 2018

This hidden checkbox is far from ideal. It's mainly here because AMP doesn't yet support a toggleClass action. See ampproject/amphtml#1032

@westonruter

This comment has been minimized.

Copy link

westonruter commented May 3, 2018

The mess I had originally proposed for using a checkbox instead of a button in AMP turned out to not be necessary, fortunately. It's much cleaner now.

@@ -58,8 +67,23 @@
</ol><!-- .comment-list -->

<?php
if ( _s_using_amp_live_list_comments() ) {
add_filter( 'navigation_markup_template', '_s_add_amp_live_list_pagination_attribute' );

This comment has been minimized.

@westonruter

westonruter May 3, 2018

This is a little awkward. It would be preferable if AMP allowed for a nav element (or role="navigation") instead of requiring an element with a custom pagination attribute.

Show resolved Hide resolved header.php
@samikeijonen

This comment has been minimized.

Copy link
Contributor

samikeijonen commented May 4, 2018

I didn't see an issue for AMP in _s so just checking in here. Looks like a nice job!

Couple of question:

  • Now that Google, Automattic and XWP are pushing AMP plugin forward, I assume that decision for supporting AMP in _s have already been made?
  • What does it actually mean. Is there going to be is_amp() checks and other AMP related stuff in template files or does this live in a different branch?
@westonruter

This comment has been minimized.

Copy link

westonruter commented May 4, 2018

Thanks!

Now that Google, Automattic and XWP are pushing AMP plugin forward, I assume that decision for supporting AMP in _s have already been made?

I don't think this decision has been made. However, in the same way that _s supports Jetpack and WooCommerce, it would seem logical to also support AMP.

What does it actually mean. Is there going to be is_amp() checks and other AMP related stuff in template files or does this live in a different branch?

See #1286 (comment). You're right that it should be opt-in. Maybe that looks like add_theme_support('amp') being commented-out by default, allowing a plugin author to enable it just by uncommenting the line?

@grappler

This comment has been minimized.

Copy link
Collaborator

grappler commented May 13, 2018

Thank you @westonruter for the pull request. It is nice to see what needs to be done on a themes side to make it compatable with the AMP Plugin.

However, in the same way that _s supports Jetpack and WooCommerce, it would seem logical to also support AMP.

As WordPress.com supports AMP and _s is used as a base for WordPress.com themes then it may be logical. I don't think it is logical for themes built by the community for clients and WordPress.org.

Jetpack support can be easily be removed be deleting the inc/jetpack.php file.
WooCommerce support is not included by default when using underscores.me.

If we were to add AMP support I would like to see easily removable.

  • Move _s_is_amp() and _s_using_amp_live_list_comments() from functions.php to it's own file e.g. inc/amp.php.
  • Create a separate comment template and load it via comments_template().
  • See if we can work with hooks in the header.php
header.php Outdated
[aria-expanded]="siteNavigationMenu.expanded ? 'true' : 'false'"
<?php endif; ?>
>
<?php esc_html_e( 'Primary Menu', '_s' ); ?>

This comment has been minimized.

@grappler

grappler May 13, 2018

Collaborator

I could not figure out what the button is for. Is it for the mobile menu?

This comment has been minimized.

@westonruter

westonruter May 13, 2018

Yes. If you resize the window to the mobile breakpoint then this button appears.

westonruter added some commits May 1, 2018

@westonruter westonruter force-pushed the xwp:amp-support branch from 6ff3728 to 5f31822 Jun 11, 2018

westonruter added some commits Jun 11, 2018

@westonruter

This comment has been minimized.

Copy link

westonruter commented Jun 11, 2018

@grappler I've simplified the PR by removing the comments_live_list support for now. I didn't think it would be good to have a separate template because then a theme author would have to maintain two separate comments templates.

Also, I've removed amp theme support from being hard-coded in functions.php entirely. This is because the AMP plugin as of 1.0-alpha1 now includes an admin screen allowing the user to decide to what kind of AMP integration they want:

image

@westonruter westonruter changed the title Try: Add AMP theme support Add AMP theme support Jun 11, 2018

@westonruter

This comment has been minimized.

Copy link

westonruter commented Jun 20, 2018

Note that WP Rig has similar built-in AMP support now as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment