Skip to content

optimizer: Output amp-geo API meta tags#636

Merged
sebastianbenz merged 3 commits intoampproject:masterfrom
mdmower:pr-geo
Mar 30, 2020
Merged

optimizer: Output amp-geo API meta tags#636
sebastianbenz merged 3 commits intoampproject:masterfrom
mdmower:pr-geo

Conversation

@mdmower
Copy link
Copy Markdown
Contributor

@mdmower mdmower commented Feb 29, 2020 •
edited
Loading

  • transformHtml paramter geoApiUrl enables amp-geo to use a fallback API
    if the AMP hosting environment does not support dynamically patching
    {{AMP_ISO_COUNTRY_HOTPATCH}} at delivery time
  • Move meta tags added by transformer before all scripts to ensure their
    content is available by the time scripts run

Depends on ampproject/amphtml#26829

@mdmower
optimizer: Output amp-geo API meta tags
fa73dc8 
- transformHtml paramter geoApiUrl enables amp-geo to use a fallback API
  if the AMP hosting environment does not support dynamically patching
  {{AMP_ISO_COUNTRY_HOTPATCH}} at delivery time
- Move meta tags added by transformer before all scripts to ensure their
  content is available by the time scripts run
@sebastianbenz
Copy link
Copy Markdown
Collaborator

sebastianbenz commented Mar 2, 2020

Looks good. One suggestion: update the ReorderHeadTransformer to move the meta tag to the beginning of. the head.

@mdmower mdmower mentioned this pull request Mar 4, 2020
Merged
@honeybadgerdontcare
Copy link
Copy Markdown

honeybadgerdontcare commented Mar 4, 2020

@sebastianbenz The CSS should always be first. Why is this tag more important than the CSS?

@sebastianbenz
Copy link
Copy Markdown
Collaborator

sebastianbenz commented Mar 4, 2020

Forget my comment. ReorderHeadTransformer does already the right thing and adds all meta tags before all script tags

https://github.com/ampproject/amp-toolbox/blob/master/packages/optimizer/lib/transformers/ReorderHeadTransformer.js#L63

@mdmower mdmower marked this pull request as ready for review March 27, 2020 23:04
@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Mar 27, 2020

@sebastianbenz This PR has been moved out of draft status and is ready for any additional review comments you might have. Thanks!

@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Mar 27, 2020

@sebastianbenz The CSS should always be first. Why is this tag more important than the CSS?

The meta tags added by this PR are inserted just before the first <script> in the head, so I don't think changes are necessary here. It would probably be worthwhile to open a new issue to ensure Optimizer orders head tags correctly in general, though.

sebastianbenz
sebastianbenz approved these changes Mar 30, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@sebastianbenz sebastianbenz merged commit 663d4b6 into ampproject:master Mar 30, 2020
@mdmower mdmower deleted the pr-geo branch March 30, 2020 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sebastianbenz sebastianbenz sebastianbenz approved these changes

Assignees

No one assigned

Labels

cla: yes enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mdmower @sebastianbenz @honeybadgerdontcare @googlebot