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 [dailymotion] shortcode AMP handling to Jetpack #14055

Merged
merged 3 commits into from
Nov 19, 2019
Merged

Migrate the [dailymotion] shortcode AMP handling to Jetpack #14055

merged 3 commits into from
Nov 19, 2019

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 16, 2019 •
edited
Loading

Summary

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

Fixes ampproject/amp-wp#3309

Changes proposed in this Pull Request:

  • Migrate [dailymotion] 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 [dailymotion] 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:
[dailymotion id=x8oma9]
  1. Preview the front-end of the AMP URL, like by adding &amp or ?ampto the URL, and look at how the dailymotion 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 dailymotion 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:
[dailymotion x8oma9 width=400 height=300]

Proposed changelog entry for your changes:

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

@kienstra
Migrate [dailymotion] shortcode support to Jetpack
b54a8e9 
Also, add tests for this.
Use the width and height calculations
in the non-AMP shortcode callback.
@kienstra kienstra requested a review from a team November 16, 2019 00:34
@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 a0ed7e0

@kienstra
Set $content_width in actual test function, not dataProvider
68ef6e8 
Tests failed in Travis,
so attempt to fix them.
@jeherve jeherve added this to the 8.0 milestone Nov 18, 2019
@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 [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
Commit Jeremy's suggestion to possibly markTestSkipped()
a0ed7e0 
This test won't apply
if it's in a WordPress.com environment.
@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 D35668-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.

Looking good now. Merging.

@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 3e5dd2f 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