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

Slideshow Block: AMP Compatibility #13009

Merged
merged 25 commits into from
Oct 3, 2019
Merged

Slideshow Block: AMP Compatibility #13009

merged 25 commits into from
Oct 3, 2019

Conversation

jeffersonrabb
Copy link
Contributor

@jeffersonrabb jeffersonrabb commented Jul 9, 2019 •
edited
Loading

Changes proposed in this Pull Request:

This PR makes the Slideshow block AMP compatible. This is achieved by using the amp-carousel element instead of Swiper in AMP requests. The functionality is similar although a few features are different and/or lost in the AMP version:

  1. No support for Fade transition style in AMP version. Transitions will always be Slides. The amp-carousel slide transition is also faster and less graceful than Swiper's.
    2) There is no Autoplay pausing in the AMP version. If Autoplay is enabled it will always be on.
    3) The pagination bullets don't show "active slide" state in the AMP version. They are functional (clicking will jump the slideshow to the requested slide) but they don't change state to indicate which is current.
    4) Dynamic height sizing will not occur in the AMP version.

Update: Autoplay pause/play button support added in cabea0d

Update: Pagination bullets active slide state in 64107c7

Update: Dynamic height sizing in 3cfa901.

Part of #9730

Testing instructions:

  • Spin up a Jurassic Ninja instance with this link: https://jurassic.ninja/create/?jetpack-beta&branch=add/amp-slideshow
  • Set up Jetpack.
  • Install and activate AMP: https://wordpress.org/plugins/amp/
  • In AMP settings, set mode to Transitional
  • Create a post, insert Slideshow block, add images.
  • Publish and view the post, then switch to AMP by interacting with the AMP item in the admin menu, or by simply appending ?amp to the URL.
  • Observe that the slideshow looks and functions like the non-AMP version (taking into account the list of differences above).

Other testing scenarios

  • Multiple slideshows on one page
  • Slideshows with captions
  • Captions with HTML tags

Proposed changelog entry for your changes:

  • Slideshow block AMP compatibility

@jeffersonrabb jeffersonrabb requested a review from a team as a code owner July 9, 2019 12:45
@jeffersonrabb jeffersonrabb self-assigned this Jul 9, 2019
@jetpackbot
Copy link

jetpackbot commented Jul 9, 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 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 9fcd5eb

@jeffersonrabb jeffersonrabb added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jul 9, 2019
westonruter
westonruter reviewed Jul 9, 2019
View reviewed changes
extensions/blocks/slideshow/slideshow.php Outdated Show resolved Hide resolved
extensions/blocks/slideshow/slideshow.php Outdated Show resolved Hide resolved
extensions/blocks/slideshow/slideshow.php Outdated Show resolved Hide resolved
extensions/blocks/slideshow/slideshow.php Outdated Show resolved Hide resolved
extensions/blocks/slideshow/slideshow.php Outdated Show resolved Hide resolved
extensions/blocks/slideshow/slideshow.php Outdated Show resolved Hide resolved
@westonruter
Copy link
Contributor

Toggling autoplay in amp-carousel: https://amp.dev/documentation/guides-and-tutorials/learn/amp-actions-and-events#amp-carousel[type=%22slides%22]_1

@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 Crew. Label will be renamed soon. labels Jul 18, 2019
@jeffersonrabb
Copy link
Contributor Author

Toggling autoplay in amp-carousel: https://amp.dev/documentation/guides-and-tutorials/learn/amp-actions-and-events#amp-carousel[type=%22slides%22]_1

Autoplay toggle UI addressed in cabea0d

@jeherve jeherve added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Sep 30, 2019
@jeffersonrabb
Copy link
Contributor Author

This works well for me. I only have some code style remarks, really.

All are addressed.

@jeffersonrabb jeffersonrabb 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 Sep 30, 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 Crew. Label will be renamed soon. labels Oct 1, 2019
@jeffersonrabb
Copy link
Contributor Author

This is not ideal, as in some languages the number may come before or within the "Go to slide" string. Do you think we could build that string earlier in another sprintf and then add it to the button at once, so translators can translate "Go to slide %d"?

This should do it: 9fcd5eb

@jeffersonrabb jeffersonrabb 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 Oct 1, 2019
jeherve
jeherve approved these changes Oct 2, 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.

This tests well for me. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 2, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeffersonrabb! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33466-code before merging this PR. Thank you!

@jeffersonrabb jeffersonrabb merged commit 4a04ac2 into master Oct 3, 2019
@jeffersonrabb jeffersonrabb deleted the add/amp-slideshow branch October 3, 2019 12:01
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 3, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13009
1384cef
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
@jeherve jeherve mentioned this pull request Nov 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@jeffersonrabb jeffersonrabb

Labels
AMP [Block] Slideshow Touches WP.com Files
Projects
None yet
Milestone
7.9
Development

Successfully merging this pull request may close these issues.
5 participants
@jeffersonrabb @jetpackbot @westonruter @matticbot @jeherve