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

AMP compat: make sure Jetpack-only functions are ignored on wpcom #12053

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Apr 15, 2019

Merges r190357-wpcom

Changes proposed in this Pull Request:

Update AMP compat file:

  • Use correct constant name
  • Add additional checks for Jetpack-only functions

Testing instructions:

  • Install the AMP plugin, and make sure you see a amp-pixel tag when viewing the site while logged out, if the Stats module is active.

Proposed changelog entry for your changes:

  • None

Merges r190357-wpcom

Update AMP compat file:

- Use correct constant name
- Add additional checks for Jetpack-only functions
@jeherve jeherve added this to the 7.3 milestone Apr 15, 2019
@jeherve jeherve requested a review from kraftbj April 15, 2019 13:02
@jeherve jeherve requested a review from a team as a code owner April 15, 2019 13:02
@jeherve jeherve self-assigned this Apr 15, 2019
@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 15, 2019
@jetpackbot
Copy link

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.

Scheduled Jetpack release: May 7, 2019.
Scheduled code freeze: April 30, 2019

Generated by 🚫 dangerJS against 9a9a48a

@jeherve jeherve merged commit bd96880 into master Apr 15, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 15, 2019
@jeherve jeherve deleted the update/amp-compat-wpcom-190357 branch April 15, 2019 13:14
jeherve added a commit that referenced this pull request Apr 24, 2019
Summary:
After adding the AMP compat file that ships with Jetpack (this was done in D22313, D26814, D26819, D26921, and D26928), let's start using this class.

This replaces D22154 and D23501, and allows us to start syncing any future changes that would rely on the Jetpack_AMP_Support class.

Related Jetpack PRs:
- #10945
- #11195, which reverted some of the changes in the PR above.
- #12054
- #12053
- #12026

Discussion:
- https://[private link]

We'll need to test for issues like the ones that popped up on Jetpack at the time:
#11169

We will also need to account for the fact that WordPress.com does not run the latest version of the AMP plugin. It runs an old version (0.6.2 vs. the current version on W.org 1.0.2), with its own version of some of the compatibility fixes that come with the class we've added (see [[ https://[private link].php | here ]]) and needs to be updated as per the discussions here:
- https://[private link] (D14521)
- https://[private link]

Test Plan:
* Create a post with a gallery
* Add a Facebook sharing button to that post.
* Try Liking that post.
* Comment on one of the images in the gallery.
* Load the post in an AMP view, and repeat the steps below.

In all cases:
- You should not see any js errors in the browser console.
- You should not get any PHP notices in your debug log.

Reviewers: zinigor

Tags: #touches_jetpack_files

Differential Revision: D26821-code

This commit syncs r190790-wpcom.
jeherve added a commit that referenced this pull request Apr 24, 2019
Summary:
After adding the AMP compat file that ships with Jetpack (this was done in D22313, D26814, D26819, D26921, and D26928), let's start using this class.

This replaces D22154 and D23501, and allows us to start syncing any future changes that would rely on the Jetpack_AMP_Support class.

Related Jetpack PRs:
- #10945
- #11195, which reverted some of the changes in the PR above.
- #12054
- #12053
- #12026

Discussion:
- https://[private link]

We'll need to test for issues like the ones that popped up on Jetpack at the time:
#11169

We will also need to account for the fact that WordPress.com does not run the latest version of the AMP plugin. It runs an old version (0.6.2 vs. the current version on W.org 1.0.2), with its own version of some of the compatibility fixes that come with the class we've added (see [[ https://[private link].php | here ]]) and needs to be updated as per the discussions here:
- https://[private link] (D14521)
- https://[private link]

Test Plan:
* Create a post with a gallery
* Add a Facebook sharing button to that post.
* Try Liking that post.
* Comment on one of the images in the gallery.
* Load the post in an AMP view, and repeat the steps below.

In all cases:
- You should not see any js errors in the browser console.
- You should not get any PHP notices in your debug log.

Reviewers: zinigor

Tags: #touches_jetpack_files

Differential Revision: D26821-code

This commit syncs r190790-wpcom.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants