Add AMP compatibility to Slideshow shortcode #15319

kienstra · 2020-04-06T20:02:15Z

Changes proposed in this Pull Request:

Adds AMP compatibility to the Slideshow shortcode
Reuses the Slideshow block render method
No change on non-AMP endpoints

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

Enhances an existing feature

Testing instructions:

Activate the AMP plugin
In AMP Settings, select Transitional mode
Create a new post
Add a Shortcode block
Enter this, with ids from your environment:

[gallery ids="1691,766,760" type="slideshow"]

The attribute type="slideshow" is what triggers the code below.

Preview the front-end of the AMP URL:

Expected: there's an <amp-carousel>, and it displays captions if the images have them:

This won't look exactly like the non-AMP endpoint, but it should have the same images.

Preview the non-AMP URL:

Checkout the master branch
Preview the non-AMP URL in another tab
Expected: the non-AMP URL looks the same with this PR and without

Proposed changelog entry for your changes:

Add AMP compatibility to Slideshow shortcode

jetpackbot · 2020-04-06T20:04:52Z

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: June 2, 2020.
Scheduled code freeze: May 26, 2020

Generated by 🚫 dangerJS against 2f4df5a

modules/shortcodes/slideshow.php

matticbot · 2020-04-16T17:00:26Z

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 D42010-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

jeherve

This works well. Reusing the work from the Slideshow block is a smart take on this. I only have one remark about this.

tests/php/modules/shortcodes/test-class.slideshow.php

…ideshow block Also, add some test cases for the AMP output, and enqueue the stylesheet for the Slideshow block.

This should help to ensure regressions, to maybe prevent wp_list_pluck() from throwing an error if there's no 'id' index.

Instead of hard-coding the image URL in the expected markup, dynamically replace it.

It looks like at least one failed Travis build is from lazy-loaded images

- Apply a consistent size and make width/height variable

MattGeri · 2020-04-30T13:52:54Z

@jeherve I've updated the tests here to use dynamic image sizes based on the size parameter passed through. I believe the parameter appended to the image URL should also be fixed now. Could you let me know if this fixes the failing tests?

The Travis Failure for e2e looks unrelated to this code.

Since the block is set to autoplay as soon as you pass an autoplay string parameter, regardless of its value, let's not pass that parameter unless necessary.

Let's not aim to validate the full slideshow markup. Instead, focus on pieces that indicate that the parameters are respected, and that the block is used.

jeherve

I've made a few changes to this PR as tests were still problematic on WordPress.com.

I cleaned up the array producing CSS classes for Gutenberg blocks, as it was previously outputting empty spaces for classes that were set as null
I reworked the autostart / autoplay conditions, as things work a bit differently between the block and the shortcode: the shortcode is set to autostart by default, while the block will not autoplay by default, but will autoplay as soon as you provide an autoplay parameter.
I simplified the tests. I don't think we need to check for the entirety of the slideshow markup; what's interesting in my opinion is whether the AMP markup is being used, and if the parameters are respected.

Let me know what you think about it!

jeherve · 2020-05-07T15:53:02Z

Hm, tests are failing on PHP 5.6 because of this:
sebastianbergmann/phpunit#2778

I'll need to work around this.

See #15319 (comment)

MattGeri · 2020-05-08T11:53:30Z

I've made a few changes to this PR as tests were still problematic on WordPress.com.

I cleaned up the array producing CSS classes for Gutenberg blocks, as it was previously outputting empty spaces for classes that were set as null

I reworked the autostart / autoplay conditions, as things work a bit differently between the block and the shortcode: the shortcode is set to autostart by default, while the block will not autoplay by default, but will autoplay as soon as you provide an autoplay parameter.

I simplified the tests. I don't think we need to check for the entirety of the slideshow markup; what's interesting in my opinion is whether the AMP markup is being used, and if the parameters are respected.

Let me know what you think about it!

LGTM! 👍

jeherve · 2020-05-14T11:03:36Z

r207455-wpcom and r207427-wpcom

kienstra requested a review from a team April 6, 2020 20:02

matticbot added the Touches WP.com Files label Apr 6, 2020

kienstra added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Apr 6, 2020

kienstra commented Apr 6, 2020

View reviewed changes

modules/shortcodes/slideshow.php Show resolved Hide resolved

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

jeherve added this to the 8.5 milestone Apr 14, 2020

jeherve requested changes Apr 16, 2020

View reviewed changes

tests/php/modules/shortcodes/test-class.slideshow.php Outdated Show resolved Hide resolved

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 Crew. Label will be renamed soon. labels Apr 16, 2020

kienstra assigned MattGeri Apr 17, 2020

kienstra mentioned this pull request Apr 18, 2020

Recipe shortcode: on AMP, change the Print link #15492

Merged

kienstra added 5 commits April 20, 2020 21:24

For the Slideshow shortcode in AMP, use the render method from the Sl…

86e6890

…ideshow block Also, add some test cases for the AMP output, and enqueue the stylesheet for the Slideshow block.

Add a test case for there being no valid ID

ecaf685

This should help to ensure regressions, to maybe prevent wp_list_pluck() from throwing an error if there's no 'id' index.

Attempt to get WordPress.com test to pass

09fbba7

Instead of hard-coding the image URL in the expected markup, dynamically replace it.

Correct typo of {{url_1}} that should have been {{url_2}

c79c58e

Account for lazy-loading images

e77a7fb

It looks like at least one failed Travis build is from lazy-loaded images

jeherve force-pushed the add/slideshow-shortcode-amp branch from 9f449fc to e77a7fb Compare April 20, 2020 19:24

jeherve removed this from the 8.5 milestone Apr 24, 2020

jeherve mentioned this pull request Apr 29, 2020

Add Image Compare block #15598

Merged

Fix unit tests

001cc63

- Apply a consistent size and make width/height variable

MattGeri added [Status] Needs Review To request a review from Crew. Label will be renamed soon. 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 May 7, 2020

jeherve added 2 commits May 7, 2020 17:11

Block classes: remove any null class values from the class array

161eb17

Slideshow block: autoplay should always be a boolean

2027997

jeherve added 2 commits May 7, 2020 17:14

Slideshow shortcode: only pass autoplay param if necessary

6e15d4c

Since the block is set to autoplay as soon as you pass an autoplay string parameter, regardless of its value, let's not pass that parameter unless necessary.

Simplify AMP tests

4aa8fdb

Let's not aim to validate the full slideshow markup. Instead, focus on pieces that indicate that the parameters are respected, and that the block is used.

jeherve previously approved these changes May 7, 2020

View reviewed changes

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 Crew. Label will be renamed soon. labels May 7, 2020

jeherve added this to the 8.6 milestone May 7, 2020

Move check for invalid IDs to its own test.

2f4df5a

See #15319 (comment)

jeherve dismissed their stale review via 2f4df5a May 7, 2020 16:05

jeherve approved these changes May 7, 2020

View reviewed changes

MattGeri approved these changes May 8, 2020

View reviewed changes

MattGeri 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 May 8, 2020

jeherve merged commit 4aac769 into master May 8, 2020

jeherve deleted the add/slideshow-shortcode-amp branch May 8, 2020 14:23

matticbot added the [Status] Needs Changelog label May 8, 2020

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels May 26, 2020

jeherve added a commit that referenced this pull request May 26, 2020

Changelog: add #15319

4b030e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AMP compatibility to Slideshow shortcode #15319

Add AMP compatibility to Slideshow shortcode #15319

kienstra commented Apr 6, 2020

jetpackbot commented Apr 6, 2020 •

edited

Loading

matticbot commented Apr 16, 2020

jeherve left a comment

MattGeri commented Apr 30, 2020

jeherve left a comment

jeherve commented May 7, 2020

MattGeri commented May 8, 2020

jeherve commented May 14, 2020 •

edited

Loading

Add AMP compatibility to Slideshow shortcode #15319

Add AMP compatibility to Slideshow shortcode #15319

Conversation

kienstra commented Apr 6, 2020

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:

Proposed changelog entry for your changes:

jetpackbot commented Apr 6, 2020 • edited Loading

matticbot commented Apr 16, 2020

jeherve left a comment

Choose a reason for hiding this comment

MattGeri commented Apr 30, 2020

jeherve left a comment

Choose a reason for hiding this comment

jeherve commented May 7, 2020

MattGeri commented May 8, 2020

jeherve commented May 14, 2020 • edited Loading

jetpackbot commented Apr 6, 2020 •

edited

Loading

jeherve commented May 14, 2020 •

edited

Loading