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.

Sign up for GitHub

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

Jump to bottom

Migrate to Jetpack the [instagram] shortcode AMP handling #14053

Merged
merged 4 commits into from
Nov 19, 2019
Merged

Migrate to Jetpack the [instagram] shortcode AMP handling #14053

merged 4 commits into from
Nov 19, 2019

Conversation

kienstra
Copy link
Contributor

Summary

Migrates to Jetpack the AMP plugin's handling of the Jetpack [instagram] shortcode

Fixes ampproject/amp-wp#3309

Changes proposed in this Pull Request:

  • Migrate [instagram] shortcode handling to Jetpack from the AMP plugin

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This is a new feature for Jetpack, but migrated from the AMP plugin

Testing instructions:

  1. Ensure that wp-config.php has define( 'JETPACK_DEV_DEBUG', true);
  2. In /wp-admin, in the Jetpack 'Settings' page, and in the 'Writing' tab, ensure this is toggled on:

jetpack-here

  1. Fetch this PR's branch
  2. Clone the AMP plugin
$ git clone --recursive https://github.com/ampproject/amp-wp.git amp && cd amp
  1. Fetch this PR of the AMP plugin, which removes the AMP plugin's [instagram] shortcode callback: Remove handling of Jetpack shortcodes ampproject/amp-wp#3678
  2. Do $ npm install && composer install
  3. Activate the AMP plugin
  4. Create a new post with a Shortcode block
  5. Add a shortcode, like:
[instagram url="https://www.instagram.com/p/B43aaRuhoCj/" width="400" height="450"]
  1. Preview the front-end of the AMP URL, like by adding &amp or ?ampto the URL, and look at how the Instagram shortcode looks
  2. checkout the master branch of Jetpack, not this PR's branch, and checkout the develop branch of the AMP plugin
  3. Preview the front-end of the AMP URL, and notice how the Instagram shortcode looks
  4. Expected: The shortcode should look similar, whether using this PR's branch, or the Jetpack plugin's master branch
  5. Test more shortcodes here

Proposed changelog entry for your changes:

Migrate AMP handling of [instagram] shortcode to Jetpack from the AMP plugin

@kienstra
Migrate to Jetpack the [instagram] embed handling from the AMP plugin
a994a75 
Also, add tests for this.
Mainly copies logic from the AMP plugin.
@kienstra kienstra requested a review from a team November 15, 2019 23:19
@jetpackbot
Copy link

jetpackbot commented Nov 15, 2019 •
edited
Loading

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: November 19, 2019.
Scheduled code freeze: November 12, 2019

Generated by 🚫 dangerJS against b03daed

@kienstra
Correct class in failed unit test
e5d7612 
This should be fallback,
not feedback.
@kienstra kienstra mentioned this pull request Nov 15, 2019
Closed
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it AMP [Feature] Shortcodes / Embeds labels Nov 18, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 18, 2019
jeherve
jeherve requested changes Nov 18, 2019
View reviewed changes
Copy link
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.

Thanks again for those.

I only have minor comments here.

tests/php/modules/shortcodes/test-class.instagram.php Show resolved Hide resolved
modules/shortcodes/instagram.php Outdated Show resolved Hide resolved
@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 Nov 18, 2019
kienstra and others added 2 commits November 18, 2019 20:14
@kienstra @jeherve
Commit Jeremy's suggestion to possibly markTestSkipped()
4eba8b5 
Co-Authored-By: Jeremy Herve <jeremy@tagada.hu>
@kienstra
Simplify the sprintf() call, using Jeremy's PR as an example
b03daed 
There should only be a need for one
string interpolation operator:
%1$s.
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! 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 Nov 19, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello kienstra! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D35663-code before merging this PR. Thank you!

jeherve
jeherve approved these changes Nov 19, 2019
View reviewed changes
Copy link
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 looks good to me. Merging.

@jeherve jeherve merged commit 4a5d42e into Automattic:master Nov 19, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 19, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
Changelog: add #14010, #14028, #14053, #14055.
1f5efa5
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
8.0 Release: running changelog (#13964)
807ab89 
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@jeherve jeherve

@kienstra kienstra

Labels
AMP [Feature] Shortcodes / Embeds Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.

Migrate Jetpack shortcode handling out of AMP plugin and into Jetpack
4 participants
@kienstra @jetpackbot @matticbot @jeherve