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 AMP compatibility for OpenTable block #17085

Merged
merged 7 commits into from
Sep 30, 2020

Conversation

westonruter
Copy link
Contributor

@westonruter westonruter commented Sep 6, 2020

See #14395

This fixes AMP compatibility for the OpenTable block by rendering an amp-iframe as opposed to the embed script on AMP pages. Note that the block edit function similarly constructs and iframe for preview purposes, so manually constructing the iframe is not unprecedented.

👉 Note this is currently blocked by an AMP bug which has been fixed upstream but is not yet in production. See ampproject/amphtml#29398. In order to test this PR, you need to first opt-in to to the beta-channel on the AMP Experiments page. The fix should be rolled out to AMP's production channel by September 22nd. The fix is now live.

Given this HTML content:

<!-- wp:jetpack/opentable {"rid":["150220"]} -->
<div class="wp-block-jetpack-opentable"><a href="https://www.opentable.com/restref/client/?rid=150220">https://www.opentable.com/restref/client/?rid=150220</a></div>
<!-- /wp:jetpack/opentable -->

<!-- wp:jetpack/opentable {"rid":["150220"],"style":"tall","className":"is-style-tall"} -->
<div class="wp-block-jetpack-opentable is-style-tall"><a href="https://www.opentable.com/restref/client/?rid=150220">https://www.opentable.com/restref/client/?rid=150220</a></div>
<!-- /wp:jetpack/opentable -->

<!-- wp:jetpack/opentable {"rid":["150220"],"style":"wide","align":"wide","className":"is-style-wide"} -->
<div class="wp-block-jetpack-opentable alignwide is-style-wide"><a href="https://www.opentable.com/restref/client/?rid=150220">https://www.opentable.com/restref/client/?rid=150220</a></div>
<!-- /wp:jetpack/opentable -->

<!-- wp:jetpack/opentable {"rid":["150220"],"style":"button","align":"","className":"is-style-button"} -->
<div class="wp-block-jetpack-opentable is-style-button"><a href="https://www.opentable.com/restref/client/?rid=150220">https://www.opentable.com/restref/client/?rid=150220</a></div>
<!-- /wp:jetpack/opentable -->

<!-- wp:paragraph -->
<p>Multi:</p>
<!-- /wp:paragraph -->

<!-- wp:jetpack/opentable {"rid":["150220","1076311"]} -->
<div class="wp-block-jetpack-opentable"><a href="https://www.opentable.com/restref/client/?rid=150220">https://www.opentable.com/restref/client/?rid=150220</a><a href="https://www.opentable.com/restref/client/?rid=1076311">https://www.opentable.com/restref/client/?rid=1076311</a></div>
<!-- /wp:jetpack/opentable -->
Non-AMP AMP Before 👎 AMP After 👍
image image image

Changes proposed in this Pull Request:

  • Add AMP compatibility for OpenTable block by using amp-iframe on AMP pages as opposed to OpenTable embed script.

Jetpack product discussion

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

Testing instructions:

  • Create a post with OpenTable blocks
  • Compare non-AMP and AMP
  • The two should be identical

Proposed changelog entry for your changes:

  • Add AMP compatibility for OpenTable block

@jetpackbot
Copy link

jetpackbot commented Sep 6, 2020

Scheduled Jetpack release: October 6, 2020.
Scheduled code freeze: September 29, 2020

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

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 1652d91

@westonruter westonruter changed the title Add initial AMP compatibility for OpenTable block Add AMP compatibility for OpenTable block Sep 6, 2020
@brbrr brbrr added [Block] OpenTable [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. AMP labels Sep 7, 2020
@jeherve jeherve added [Status] Blocked / Hold and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 10, 2020
@westonruter westonruter mentioned this pull request Sep 15, 2020
30 tasks
@westonruter westonruter force-pushed the add/amp-opentable-compat branch from 26016d2 to aadba1d Compare September 23, 2020 18:07
@westonruter westonruter marked this pull request as ready for review September 23, 2020 18:07
@westonruter
Copy link
Contributor Author

@jeherve No longer [Status] Blocked / Hold.

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Blocked / Hold labels Sep 24, 2020
@jeherve jeherve added this to the 9.0 milestone Sep 24, 2020
@jeherve jeherve self-assigned this Sep 24, 2020
@jeherve
Copy link
Member

jeherve commented Sep 24, 2020

Internal reference: D50129-code

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 24, 2020
@westonruter westonruter force-pushed the add/amp-opentable-compat branch from aadba1d to f0b54e1 Compare September 24, 2020 21:02
@jeherve jeherve removed this from the 9.0 milestone Sep 28, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 30, 2020
@jeherve jeherve added this to the 9.1 milestone Sep 30, 2020
@jeherve jeherve merged commit 4f371f5 into Automattic:master Sep 30, 2020
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 30, 2020
jeherve added a commit that referenced this pull request Oct 6, 2020
jeherve added a commit that referenced this pull request Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP [Block] OpenTable Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants