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 some of the AMP [soundcloud] shortcode conversion to Jetpack #14028

Merged
merged 5 commits into from
Nov 19, 2019
Merged

Migrate some of the AMP [soundcloud] shortcode conversion to Jetpack #14028

merged 5 commits into from
Nov 19, 2019

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 14, 2019 •
edited
Loading

Summary

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

Fixes ampproject/amp-wp#3309

Changes proposed in this Pull Request:

  • Migrate [soundcloud] shortcode handling to Jetpack from the AMP plugin
  • No intended change to the non-AMP behavior

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 [soundcloud] 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:
[soundcloud url="https://soundcloud.com/closetorgan/sets/smells-like-lynx-africa-private"]
  1. Preview the front-end of the AMP URL, like by adding &amp or ?ampto the URL, and look at how the Soundcloud 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 Soundcloud 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 some AMP-conversion of [soundcloud] shortcode to Jetpack from the AMP plugin

@kienstra
Migrate some of the AMP [soundcloud] shortcode handling to Jetpack
508dec6 
The AMP plugin allows more $url values.
For example, it allows:
[soundcloud https://soundcloud.com/bonifansius/example-track]
Whereas Jetpack's non-AMP handling only allows
[souncloud url=<url here>].
@see https://github.com/ampproject/amp-wp/blob/e798258c22c465d60b4bd259f31d32c52817d6c7/includes/embeds/class-amp-soundcloud-embed.php#L128
@kienstra
Copy a comment from the AMP plugin to this
fe5dc6a 
This is important to explain
where the conversion will happen.
@kienstra kienstra requested a review from a team November 14, 2019 06:13
@jetpackbot
Copy link
Collaborator

jetpackbot commented Nov 14, 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 bdb5b55

@kienstra kienstra mentioned this pull request Nov 14, 2019
Merged
3 tasks
@kienstra
Copy link
Contributor Author

This is a lot simpler than previous shortcode PRs, as an oEmbed handler in the AMP plugin handles much of the conversion.

@kienstra kienstra mentioned this pull request Nov 14, 2019
Closed
@jeherve jeherve added [Feature] Shortcodes / Embeds [Pri] Normal [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 labels Nov 15, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 15, 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.

I just have a small suggestion.

Do you think you could also add a Unit Test to cover this scenario?

Thank you!

@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! [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. 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 3 commits November 18, 2019 20:59
@kienstra @jeherve
Commit Jeremy's suggestion to check for the URL being empty
62695ad 
Co-Authored-By: Jeremy Herve <jeremy@tagada.hu>
@kienstra
Add a unit test for the AMP [soundcloud] logic
328a88c 
This should run only if it's an AMP endpoint,
and if the URL is not empty().
@kienstra
Skip this test on wp.com environments, copying Jeremy's snippet
bdb5b55 
Use Jeremy's snippet that we've used on
other shortcode PRs.
@kienstra
Copy link
Contributor Author

Hi @jeherve,
Good point about a unit test. How does the unit test in the last 2 commits look?

@jeherve jeherve removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. 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 D35669-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.

Good as well. 🚢

@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 19, 2019
@jeherve jeherve merged commit 9e5caee 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 [Pri] Normal 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