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.

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

Carousel: Check if image parent is not a link #6647

Merged
merged 2 commits into from
Mar 14, 2017

Conversation

stoyan0v
Copy link
Contributor

Fixes #6638

Changes proposed in this Pull Request:

  • Add additional condition to check if the parent is not a link, to prevent javascript errors.

Testing instructions:

  • Enable Carousel
  • Disable html5 support for captions
  • Create new page and add a sample gallery
  • Add two or more caption shortcodes wrapped in a custom a tag (you can use the following code )
  • Open the page and make sure that there is no javascript error like the following :
    Uncaught TypeError: Cannot read property 'split' of undefined
    On line 451 of modules/carousel/jetpack-carousel.js

cc @jeherve

@jeherve jeherve requested review from jeherve and zinigor March 14, 2017 11:54
@jeherve jeherve added [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Pri] BLOCKER [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended labels Mar 14, 2017
@jeherve jeherve added this to the 4.7.1 milestone Mar 14, 2017
@@ -445,6 +445,11 @@ jQuery(document).ready(function($) {
return;
}

// skip if the parent is not a link
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but one tiny nitpick. We're not talking about the parent here, but about the container, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no 😄 . The container is actually the parent, but I will change this if you want.

@zinigor
Copy link
Member

zinigor commented Mar 14, 2017

OK, so I guess the container.parent above was the grandparent :) Thanks, looks good!

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 14, 2017
jeherve added a commit that referenced this pull request Mar 14, 2017
@dereksmart dereksmart merged commit 7f18cdb into Automattic:master Mar 14, 2017
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 14, 2017
dereksmart pushed a commit that referenced this pull request Mar 14, 2017
* Carousel: Check if the image parent is not a link

* Change 'parent' to 'container' in comment
@dereksmart
Copy link
Member

merged to 4.7 eae9b8f

dereksmart pushed a commit that referenced this pull request Mar 14, 2017
* Changelog: add #6641

* Changelog: add #6647

* Changelog: add #6631

* Changelog: add #6630

* Changelog: update release date.
dereksmart pushed a commit that referenced this pull request Mar 14, 2017
* Changelog: add #6641

* Changelog: add #6647

* Changelog: add #6631

* Changelog: add #6630

* Changelog: update release date.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Pri] BLOCKER [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants