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 the [tweet] shortcode AMP handling to Jetpack #14054

Merged
merged 5 commits into from
Nov 19, 2019
Merged

Migrate the [tweet] shortcode AMP handling to Jetpack #14054

merged 5 commits into from
Nov 19, 2019

Conversation

kienstra
Copy link
Contributor

Summary

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

Fixes ampproject/amp-wp#3309

Changes proposed in this Pull Request:

  • Migrate [tweet] 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 'Compose using shortcodes...' is toggled on:

  3. Fetch this PR's branch

  4. 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 [tweet] 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:
[tweet https://twitter.com/jack/statuses/20 width="350"]
  1. Preview the front-end of the AMP URL, like by adding &amp or ?ampto the URL, and look at how the tweet 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 tweet 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, like:
[tweet 20]
[tweet https://twitter.com/photomatt/status/1191968085093343234]

Proposed changelog entry for your changes:

Migrate to Jetpack the AMP handling of the [tweet] shortcode

@kienstra
Migrate [tweet] shortcode AMP conversion to AMP
757ec4b 
This mainly copies the logic in the
AMP plugin into Jetpack.
@kienstra
Add a test case for width=0
5dc937d 
In that case, the width
should simply be the $default_width.
@kienstra
Remove extra variable $initial_width
c85313f 
There's probably no need to run absint() first,
so simply set the $width.
@kienstra kienstra requested a review from a team November 16, 2019 00:04
@kienstra kienstra mentioned this pull request Nov 16, 2019
Closed
@jetpackbot
Copy link

jetpackbot commented Nov 16, 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 37c1df7

@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
tests/php/modules/shortcodes/test-class.tweet.php Show resolved Hide resolved
modules/shortcodes/tweet.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:28
@kienstra @jeherve
Commit Jeremy's suggestion to possibly markTestSkipped()
46a25ca 
Co-Authored-By: Jeremy Herve <jeremy@tagada.hu>
@kienstra
Move the 'wp_footer' hook to an else block
37c1df7 
As Jeremy mentioned,
this should not run for AMP endpoints.
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.

LGTM. Merging.

@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 D35666-code before merging this PR. Thank you!

@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
@jeherve jeherve merged commit 2479f65 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 #14054
8239ad0
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