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

Add preload links & resource hints, and optimize order of elements in head #1295

Merged
merged 5 commits into from Aug 4, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jul 31, 2018

In lieu of WordPress not yet supporting preload links (see #42438) and not always supporting resource hints (see #44668), we can use the fact that we have a DOMDocument to supply them when missing.

This seeks to implement much of the drafted AMP hosting guide.

  • Add preload links for AMP runtime and render-blocking extension scripts.
  • Ensure preconnect resource link is added for Google Fonts.
  • Optimize order of elements in head, including putting style[amp-custom] before external external stylesheets and place boilerplate after style[amp-custom].

In my testing, the changes in this PR improve the Lighthouse Performance score on my dev environment for Twenty Sixteen by 2 points: 78 to 80.

image

Note that this PR does not add preload links for images (i.e. header image and featured image since these are theme-specific; this needs to be done in core/ see #42438.)

See also #1289.

For optimizing fonts other than from Google Fonts, see ampproject/amp-toolbox#76

Closes #1265.

@westonruter westonruter added this to the v1.0 milestone Jul 31, 2018
@westonruter westonruter force-pushed the add/preload-and-resource-hints branch from 1b87ed5 to 57defe2 Compare August 3, 2018 18:56
@westonruter westonruter force-pushed the add/preload-and-resource-hints branch from 0559edb to d09034d Compare August 4, 2018 04:16
@westonruter westonruter changed the title [WIP] Add preload links and resource hints Add preload links and resource hints Aug 4, 2018
@westonruter westonruter changed the title Add preload links and resource hints Add preload links & resource hints, and optimize order of elements in head Aug 4, 2018
* in this case too we should defer to the theme as well to output the meta charset because it is possible the
* install is not on utf-8 and we may need to do a encoding conversion.
* "AMP HTML documents MUST contain the AMP boilerplate code (head > style[amp-boilerplate] and noscript > style[amp-boilerplate]) in their head tag."
* https://www.ampproject.org/docs/fundamentals/spec#required-markup
Copy link
Contributor

Choose a reason for hiding this comment

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

@westonruter What do you think about standardizing our inline link references to this:

"AMP HTML documents MUST contain the AMP boilerplate code (head > style[amp-boilerplate] and 
noscript > style[amp-boilerplate]) in their head tag." {@link https://www.ampproject.org/docs/fundamentals/spec#required-markup AMP Required Markup}.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine with me, but at the same time since this is just a multiline comment and not a phpDoc /** comment, it wouldn't make a difference from an automated documentation generator perspective AFAIK.

Copy link
Contributor

Choose a reason for hiding this comment

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

@westonruter True. I'm more thinking about consistency of how we document links whether in a DocBlock, file header, or inline comment. We have a mixture of documenting techniques:

  • {@see http Document Title}
  • See http....
  • see <http....>
  • etc.

This is a larger discussion on our coding standards to ensure we have consistency.

$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0/amp-mathml-latest.js" async custom-element="amp-mathml"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
$this->assertContains( '<meta name="generator" content="AMP Plugin', $sanitized_html );

$ordered_contains = array(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

Good work. 👍

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Aug 4, 2018

@westonruter We do need to remember to update all links to the draft AMP Hosting Guide once it's finalized and published.

@hellofromtonya hellofromtonya merged commit b074286 into develop Aug 4, 2018
@westonruter westonruter deleted the add/preload-and-resource-hints branch September 6, 2018 03:15
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.

None yet

2 participants