Skip to content

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

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

Migrate some of the AMP [soundcloud] shortcode conversion to Jetpack#14028
jeherve merged 5 commits into
Automattic:masterfrom
kienstra:add/soundcloud-amp-support

Conversation

@kienstra
Copy link
Copy Markdown
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
Copy Markdown
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 commented Nov 14, 2019
View reviewed changes
Comment thread modules/shortcodes/soundcloud.php
kienstra
kienstra commented Nov 14, 2019
View reviewed changes
Comment thread modules/shortcodes/soundcloud.php
@kienstra kienstra mentioned this pull request Nov 14, 2019
Merged
3 tasks
@kienstra
Copy link
Copy Markdown
Contributor Author

kienstra commented Nov 14, 2019

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 This PR is ready for review. 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
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.

I just have a small suggestion.

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

Thank you!

Comment thread modules/shortcodes/soundcloud.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. [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. and removed [Status] Needs Review This PR is ready for review. 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
Copy Markdown
Contributor Author

kienstra commented Nov 19, 2019

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 need more details from you. This label will be auto-added until the PR meets all requirements. [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. labels Nov 19, 2019
@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 D35669-code before merging this PR. Thank you!

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.

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 Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Shortcodes / Embeds [Pri] Normal 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