Skip to content

Fix AMP compatibility for Google Calendar block#17251

Merged
jeherve merged 3 commits intoAutomattic:masterfrom
westonruter:add/google-calendar-amp-compat
Sep 24, 2020
Merged

Fix AMP compatibility for Google Calendar block#17251
jeherve merged 3 commits intoAutomattic:masterfrom
westonruter:add/google-calendar-amp-compat

Conversation

@westonruter
Copy link
Copy Markdown
Contributor

See #14395

Given this post_content:

<!-- wp:separator -->
<hr class="wp-block-separator"/>
<!-- /wp:separator -->

<!-- wp:jetpack/google-calendar {"url":"https://calendar.google.com/calendar/embed?src=askenergy%40oregon.gov\u0026ctz=America/Los_Angeles"} -->
<a href="https://calendar.google.com/calendar/embed?src=askenergy%40oregon.gov&amp;ctz=America/Los_Angeles" class="wp-block-jetpack-google-calendar">https://calendar.google.com/calendar/embed?src=askenergy%40oregon.gov&amp;ctz=America/Los_Angeles</a>
<!-- /wp:jetpack/google-calendar -->

<!-- wp:separator -->
<hr class="wp-block-separator"/>
<!-- /wp:separator -->

Google Calendar is currently not rendering at all on an AMP page. This is because it is getting removed due to not having a width defined on the amp-iframe:

image

This PR fixes that issue by supplying the proper layout of fixed-height rather than responsive, since the width is intended to be 100%. (This appears to be a regression introduced in #14835.) Also missing in the previous implementation was the placeholder child which is required for amp-iframe elements appearing in the initial viewport. So a link to the Google Calendar was supplied as a placeholder in this case.

This PR also goes beyond just fixing AMP compatibility in that the page is actually better than the non-AMP version when JS is turned off, by supplying a the plain HTML calendar view to visitors with JS turned off. Some additional AMP-specific CSS was included here, but that will eventually not be needed once those are accounted for in AMP itself. See ampproject/amphtml#29846.

Before
Non-AMP w/ JS Non-AMP w/o JS AMP 👎 AMP w/o JS 👎
image image image (Removed by plugin sanitizer) image (Removed by plugin sanitizer)
After
Non-AMP w/ JS Non-AMP w/o JS AMP 👍 AMP w/o JS 👍
image (No change) image (No change) image image (No-JS fallback!)

Changes proposed in this Pull Request:

  • Fix AMP compatibility in Google Calendar block.

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  1. Copy the above post_content into a new post.
  2. Activate the AMP plugin and enable Transitional mode.
  3. Compare the post in AMP and non-AMP (e.g. via Paired Browsing).

Proposed changelog entry for your changes:

  • Fix AMP compatibility in Google Calendar block.

@jetpackbot
Copy link
Copy Markdown
Collaborator

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17251

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against b6475ff

@jeherve jeherve added AMP Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [Pri] Normal labels Sep 24, 2020
@jeherve jeherve added this to the 9.0 milestone Sep 24, 2020
@scruffian scruffian requested a review from a team September 24, 2020 12:45
@jeherve jeherve self-assigned this Sep 24, 2020
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me. Merging.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 24, 2020
@jeherve jeherve merged commit 6cc73b6 into Automattic:master Sep 24, 2020
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 24, 2020
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Sep 24, 2020

r214186-wpcom

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

Labels

AMP [Block] Google Calendar Bug When a feature is broken and / or not performing as intended [Pri] Normal Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants