Migrate the vimeo shortcode AMP logic to Jetpack from the AMP plugin #13921

kienstra · 2019-10-31T23:28:55Z

Summary

Proposes migrating the AMP conversion of the Jetpack [vimeo] shortcode from the AMP plugin to Jetpack.

Changes proposed in this Pull Request:

Would you be open to migrating the AMP plugin's conversion of Jetpack's [vimeo] shortcode into Jetpack?

This should give you more control over how your shortcode works.

As Weston mentioned in ampproject/amp-wp#3309 (comment), there's been ongoing work to move Jetpack functionality from the AMP plugin into Jetpack.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is a new feature to Jetpack, but mainly a migration of the AMP plugin's logic into Jetpack.

Testing instructions:

Ensure that wp-config.php has define( 'JETPACK_DEV_DEBUG', true);
In /wp-admin, in the Jetpack 'Settings' page, and in the 'Writing' tab, ensure this is toggled on:

Fetch this PR's branch
Clone the AMP plugin

$ git clone --recursive https://github.com/ampproject/amp-wp.git amp && cd amp

Fetch this PR of the AMP plugin, which deactivates the AMP plugin's [vimeo] shortcode callback: Remove handling of Jetpack shortcodes ampproject/amp-wp#3678
Do $ npm install && composer install
Activate the AMP plugin
Create a new post with a Shortcode block
Add a shortcode, like:

[vimeo id="84618094" width="600" height="400"]

Preview the front-end of the AMP URL, like by adding &amp or ?ampto the URL, and look at how the Vimeo shortcode looks
checkout the master branch of Jetpack, not this PR's branch
Preview the front-end of the AMP URL, and notice how the Vimeo shortcode looks
Expected: The shortcode should look the same, whether using this PR's branch, or the Jetpack plugin's master branch
Here are some more shortcodes to test:

[vimeo id="84618094"]
[vimeo 8461809]
[vimeo id="84618094" width="600"]
[vimeo id="84618094" width="400" height="300"]
[vimeo https://vimeo.com/84618094]

Before

After

Proposed changelog entry for your changes:

Migrate AMP-conversion of Vimeo shortcode from the AMP plugin to Jetpack.

Most of the functionality is the same, though there are some changes. Also, add 2 parameters to the filter 'video_embed_html'. This might be questionable, but it prevents adding another filter, or adding AMP logic directly in the Vimeo shortcode file.

jetpackbot · 2019-10-31T23:32:08Z

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: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against 2344248

These functions were 2 and 1 lines long, respectively.

In response to Code Climate report, with the cognitive complexity too high.

In response to a Code Climate report of the class being too long.

Also, add it to the phpcs whitelist to be linted.

Instead of onthe, change this to on the. Also, front-end instead of frontend.

kienstra · 2019-11-01T00:44:18Z

With the commits above, this should be ready for review. But no pressure, this is a busy week 😄

chvillanuevap · 2019-11-01T02:42:29Z

Does this work with just [vimeo 84618094]? Because that's supported by the current shortcode in Jetpack.

This will then use 12345 as the data-videoid. For compatibility with how the non-AMP shortcode works, though this didn't work in AMP before.

kienstra · 2019-11-01T17:56:50Z

Hi @chvillanuevap, it does now 😄

In case there are more shortcode handlers. The Code Climate limit of around 200 LOC per class won't allow for more than 1 more shortcode handler in this class.

This is only called by build_tag(), so it shouldnt' be needed.

There were apparently 5 too many LOC, so this reduces the lines.

The name of $string is misleading, as it's actually an array().

This file name changed, so change it in the whitelist also.

This is in a class with Vimeo in the name, so it's probably not necessary to have Vimeo in the function name.

This should have been almost at the top before, so move it one place up.

It looks like array() is by far the most common syntax in Jetpack.

Uses jetpack_shortcode_get_vimeo_dimensions() for both the non-AMP and AMP shortcodes.

The means of getting dimension changed, so update the tests.

The function name is no jetpack_amp_vimeo_shortcode in Automattic/jetpack#13921

There were still a few places where the new name of jetpack_amp_vimeo_shortcode() did not appear.

@covered

That has its own tests, so it's not very helpful to mark that as @covered.

There was only 1 failure: rbenv: version 7.2 not installed

kienstra · 2019-11-06T20:02:53Z

Hi Jeremy,
Thanks for your diff above, I committed it verbatim. Also, I made a few changes, like updating tests.

All of the testing shortcodes in step 13 here look good with AMP.

They don't all look exactly the same as the non-AMP shortcodes, as the <amp-vimeo> has a layout="responsive", meaning that it will expand to fill its container. So sometimes it will look bigger than the non-AMP shortcode.

That was also the case in the AMP plugin.

jeherve

This looks good to me. It should be good to merge.

After thinking about this some more, I committed 1bfc3e6 to simplify things a bit more. Now that we moved dimensions into their own function, I think that makes things a bit simpler and easier to follow. It should also be a bit better performance-wise.

matticbot · 2019-11-07T11:19:00Z

kienstra, Your synced wpcom patch D35016-code has been updated.

The AMP plugin is not up to date on WordPress.com yet, we can't test everything there yet.

* 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

kienstra requested a review from a team October 31, 2019 23:28

matticbot added Touches WP.com Files labels Oct 31, 2019

kienstra mentioned this pull request Oct 31, 2019

Migrate Jetpack shortcode handling out of AMP plugin and into Jetpack ampproject/amp-wp#3309

Closed

kienstra added 3 commits October 31, 2019 17:34

In response to Code Climate, shorten 2 functions

c7b39e5

These functions were 2 and 1 lines long, respectively.

Abstract part of filter_vimeo_shortcode() into another function

5b0c942

In response to Code Climate report, with the cognitive complexity too high.

Move the shortcode logic into a new class

59fbb51

In response to a Code Climate report of the class being too long.

matticbot removed the Touches WP.com Files label Nov 1, 2019

Add new file to the files to be required

20a701d

Also, add it to the phpcs whitelist to be linted.

matticbot added the Touches WP.com Files label Nov 1, 2019

Update the PHP DocBlocks, correct typos

e24d28e

Instead of onthe, change this to on the. Also, front-end instead of frontend.

Handle a shortcode of [vimeo 12345]

03d11b7

This will then use 12345 as the data-videoid. For compatibility with how the non-AMP shortcode works, though this didn't work in AMP before.

kienstra added 10 commits November 1, 2019 12:08

Rename Jetpack_AMP_Shortcodes to Jetpack_AMP_Vimeo_Shortcode

4792db5

In case there are more shortcode handlers. The Code Climate limit of around 200 LOC per class won't allow for more than 1 more shortcode handler in this class.

In response to Code Climate, remove build_attributes_string()

7751f6e

This is only called by build_tag(), so it shouldnt' be needed.

Reduce lines of code in response to Code Climate

18a9db0

There were apparently 5 too many LOC, so this reduces the lines.

Rename $string to $collected_attributes

1e877c5

The name of $string is misleading, as it's actually an array().

Empty commit to trigger another Travis build

18863c1

Merge branch 'master' into add/vimeo-shortcode-amp

3e601b7

In response to failed Travis build, change file in whitelist

db5ac8f

This file name changed, so change it in the whitelist also.

Rename filter_vimeo_shortcode() to filter_shortcode()

949de5c

This is in a class with Vimeo in the name, so it's probably not necessary to have Vimeo in the function name.

Move test_init() above a dataProvider for a lower function

cf4342d

This should have been almost at the top before, so move it one place up.

Replace [] syntax for arrays with array()

86ca9c1

It looks like array() is by far the most common syntax in Jetpack.

jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it AMP [Feature] Shortcodes / Embeds labels Nov 4, 2019

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 6, 2019

kienstra added 2 commits November 6, 2019 12:36

Commit Jeremy's diff

d874efe

Uses jetpack_shortcode_get_vimeo_dimensions() for both the non-AMP and AMP shortcodes.

Update unit tests for the previous commit

9330915

The means of getting dimension changed, so update the tests.

kienstra added a commit to ampproject/amp-wp that referenced this pull request Nov 6, 2019

Change function name to reflect the change in the Jetpack PR

0192ff5

The function name is no jetpack_amp_vimeo_shortcode in Automattic/jetpack#13921

kienstra added 3 commits November 6, 2019 13:25

Update the name of the function in the test files

451ef9f

There were still a few places where the new name of jetpack_amp_vimeo_shortcode() did not appear.

Remove @Covers tag for jetpack_shortcode_get_vimeo_dimensions()

ee38ebc

That has its own tests, so it's not very helpful to mark that as @covered.

Empty commit to trigger Travis build

a485777

There was only 1 failure: rbenv: version 7.2 not installed

jeherve removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 7, 2019

Move AMP rendering into main shortcode function

1bfc3e6

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 7, 2019

jeherve previously approved these changes Nov 7, 2019

View reviewed changes

Update tests to match 1bfc3e6

d0fac0c

jeherve dismissed their stale review via d0fac0c November 7, 2019 11:37

Skip some tests on WordPress.com.

2344248

The AMP plugin is not up to date on WordPress.com yet, we can't test everything there yet.

jeherve approved these changes Nov 7, 2019

View reviewed changes

jeherve merged commit bba41df into Automattic:master Nov 7, 2019

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 7, 2019

kienstra deleted the add/vimeo-shortcode-amp branch November 7, 2019 19:14

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Nov 8, 2019

jeherve added a commit that referenced this pull request Nov 8, 2019

Changelog: add #13921

92d98ee

kienstra mentioned this pull request Nov 11, 2019

Move AMP rendering of [youtube] shortcode to Jetpack from AMP plugin #14010

Merged

jeherve added a commit that referenced this pull request Nov 15, 2019

Changelog: add #13921

209e05c

JoshuaGoode mentioned this pull request Dec 26, 2019

Vimeo Shortcode Embed: Broken Formatting after Recent Update #14279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the vimeo shortcode AMP logic to Jetpack from the AMP plugin #13921

Migrate the vimeo shortcode AMP logic to Jetpack from the AMP plugin #13921

kienstra commented Oct 31, 2019 •

edited

Loading

jetpackbot commented Oct 31, 2019 •

edited

Loading

kienstra commented Nov 1, 2019

chvillanuevap commented Nov 1, 2019

kienstra commented Nov 1, 2019

kienstra commented Nov 6, 2019 •

edited

Loading

jeherve left a comment

matticbot commented Nov 7, 2019

Migrate the vimeo shortcode AMP logic to Jetpack from the AMP plugin #13921

Migrate the vimeo shortcode AMP logic to Jetpack from the AMP plugin #13921

Conversation

kienstra commented Oct 31, 2019 • edited Loading

Summary

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Before

After

Proposed changelog entry for your changes:

jetpackbot commented Oct 31, 2019 • edited Loading

kienstra commented Nov 1, 2019

chvillanuevap commented Nov 1, 2019

kienstra commented Nov 1, 2019

kienstra commented Nov 6, 2019 • edited Loading

jeherve left a comment

Choose a reason for hiding this comment

matticbot commented Nov 7, 2019

kienstra commented Oct 31, 2019 •

edited

Loading

jetpackbot commented Oct 31, 2019 •

edited

Loading

kienstra commented Nov 6, 2019 •

edited

Loading