Skip to content

Ensure all AMP scripts (including v0.js) get moved to the head#3334

Merged
westonruter merged 1 commit intodevelopfrom
fix/moving-v0js-to-head
Sep 24, 2019
Merged

Ensure all AMP scripts (including v0.js) get moved to the head#3334
westonruter merged 1 commit intodevelopfrom
fix/moving-v0js-to-head

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Sep 23, 2019

When using a Latest Stories block on a page, this results on amp-carousel and amp-runtime scripts being enqueued on the page, and printed in the footer. While AMP component scripts are getting moved to the head, this was not being done for v0.js. So this ensures that no validation error from happening regarding v0.js being in the body:

image

Steps to reproduce:

  1. Create a story.
  2. Create a page containing a Latest Stories block.
  3. View the page as AMP.
  4. Without the fix, a validation error should be reported (above); with the fix, no error should be reported.

@westonruter westonruter added this to the v1.3 milestone Sep 23, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Sep 23, 2019
@westonruter westonruter added Size: XS and removed cla: yes Signed the Google CLA labels Sep 23, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Sep 23, 2019
@westonruter westonruter mentioned this pull request Sep 23, 2019
7 tasks
Copy link
Copy Markdown
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @westonruter,
This looks good.

Like you mentioned, before, v0.js appeared in the <body> on AMP endpoints:

validation-error-body

...but now it appears in the <head>:

v0-in-h

* @covers AMP_Theme_Support::prepare_response()
* @covers AMP_Theme_Support::ensure_required_markup()
*/
public function test_scripts_get_moved_to_head() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice testing of this!

@westonruter westonruter merged commit b2b6e0b into develop Sep 24, 2019
@westonruter westonruter deleted the fix/moving-v0js-to-head branch September 24, 2019 00:32
westonruter added a commit that referenced this pull request Sep 24, 2019
@swissspidy
Copy link
Copy Markdown
Collaborator

Verified in QA

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

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants