Skip to content

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

Merged
jeherve merged 5 commits into
Automattic:masterfrom
kienstra:add/tweet-shortcode-amp
Nov 19, 2019
Merged

Migrate the [tweet] shortcode AMP handling to Jetpack#14054
jeherve merged 5 commits into
Automattic:masterfrom
kienstra:add/tweet-shortcode-amp

Conversation

@kienstra
Copy link
Copy Markdown
Contributor

@kienstra kienstra commented Nov 16, 2019

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 commented Nov 16, 2019
View reviewed changes
Comment thread modules/shortcodes/tweet.php Outdated
kienstra
kienstra commented Nov 16, 2019
View reviewed changes
Comment thread tests/php/modules/shortcodes/test-class.tweet.php
@kienstra kienstra mentioned this pull request Nov 16, 2019
Closed
@jetpackbot
Copy link
Copy Markdown
Collaborator

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 This PR is ready for review. 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
Comment thread tests/php/modules/shortcodes/test-class.tweet.php
Comment thread modules/shortcodes/tweet.php Outdated
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. 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
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.

LGTM. Merging.

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Nov 19, 2019

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 need more details from you. This label will be auto-added until the PR meets all requirements. 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 Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Shortcodes / Embeds Touches WP.com Files

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