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

Carousel: Fix markup changes from Gutenberg 6.5. #13582

Merged
merged 3 commits into from
Oct 1, 2019
Merged

Carousel: Fix markup changes from Gutenberg 6.5. #13582

merged 3 commits into from
Oct 1, 2019

Conversation

kwight
Copy link
Contributor

@kwight kwight commented Sep 27, 2019 •
edited
Loading

Gutenberg 6.5 introduced gallery captions, which also took the liberty of making some markup changes; these changes removed markup that Carousel depends on for its click handlers. This PR updates the class Carousel looks for to identify the new Gallery blocks.

See: Automattic/wp-calypso#36406

Testing instructions:

  • Load your Jetpack dev environment with the Gutenberg 6.5 plugin active, SCRIPT_DEBUG defined, and the Carousel module active.
  • Create a new post with the block editor, add a Gallery block, and select "Media file" under the "Link To" dropdown in the block's sidebar.
  • Publish the post, and verify on the front-end that the Carousel does not display when a gallery image is clicked.
  • Switch to this PR.
  • Refresh the published post, and verify the Carousel now works as expected.

Proposed changelog entry for your changes:

  • Bug: Fix carousels when the Gutenberg v6.5.0 plugin is active.

@kwight kwight requested a review from a team September 27, 2019 22:36
@kwight kwight self-assigned this Sep 27, 2019
@kwight
Copy link
Contributor Author

kwight commented Sep 27, 2019

cc/ @jeherve after noticing your ongoing work with the carousel :)

@jetpackbot
Copy link

jetpackbot commented Sep 27, 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: October 1, 2019.
Scheduled code freeze: September 24, 2019

Generated by 🚫 dangerJS against a72e52c

@kwight kwight added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 27, 2019
@kwight kwight mentioned this pull request Sep 27, 2019
Closed
@kwight
Copy link
Contributor Author

kwight commented Sep 27, 2019

Oh dang release cycles; @jeherve any hope of getting this cherry-picked for 7.8? <3

@kwight
Copy link
Contributor Author

kwight commented Sep 28, 2019

Hm, maybe this should be just .blocks-gallery-grid – related: https://make.wordpress.org/core/2019/09/27/block-editor-theme-related-updates-in-wordpress-5-3/

@jeherve jeherve added this to the 7.9 milestone 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! [Type] Bug When a feature is broken and / or not performing as intended [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 30, 2019
@jeherve
Copy link
Member

jeherve commented Sep 30, 2019 •
edited
Loading

any hope of getting this cherry-picked for 7.8? <3

I think it's fine if it makes it in 7.9. Next month's release will land before WordPress 5.3, so we should be okay.

That shouldn't stop from porting the fix over to WordPress.com as soon as it is merged here in Jetpack though; we can fix the problem there quicker that way.

jeherve
jeherve requested changes Sep 30, 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.

Quick note, as I think we should keep supporting the old version as well, at least for a bit.

modules/carousel/jetpack-carousel.php Show resolved Hide resolved
@jeherve jeherve mentioned this pull request Sep 30, 2019
Closed
11 tasks
@kwight
Copy link
Contributor Author

kwight commented Sep 30, 2019

I think it's fine if it makes it in 7.9. Next month's release will land before WordPress 5.3, so we should be okay.

Coolio 👍

That shouldn't stop from porting the fix over to WordPress.com as soon as it is merged here in Jetpack though; we can fix the problem there quicker that way.

I can't get a clean diff auto-generated, and I'm imagining it's from the 20 or so wpcom-specific hacks we have in jetpack-carousel.js – does that mean I do the force-override on the MC run-fusion page to get this green here?

@kwight kwight added [Status] Needs Review To request a review from fellow Jetpack developers. 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
@kwight
Copy link
Contributor Author

kwight commented Sep 30, 2019

D33335-code for the WP.com patch.

@kwight kwight mentioned this pull request Sep 30, 2019
Closed
@kwight
Copy link
Contributor Author

kwight commented Sep 30, 2019

D33335-code has been deployed due to high user reports.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Tested on WP.com and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 1, 2019
jeherve
jeherve approved these changes Oct 1, 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 works well in my tests. Merging.

@jeherve jeherve merged commit 51e18c0 into master Oct 1, 2019
@jeherve jeherve deleted the fix/carousel-gallery-block-markup branch October 1, 2019 09:56
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 1, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13582
bd0c152
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbrown9 kbrown9 kbrown9 left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@kwight kwight

Labels
[Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Status] Tested on WP.com Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
7.9
Development

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