Skip to content

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

Merged
dereksmart merged 2 commits intoAutomattic:masterfrom
stoyan0v:fix/carousel/gallery-error
Mar 14, 2017
Merged

Carousel: Check if image parent is not a link#6647
dereksmart merged 2 commits intoAutomattic:masterfrom
stoyan0v:fix/carousel/gallery-error

Conversation

@stoyan0v
Copy link
Copy Markdown
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 This PR is ready for review. 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
return;
}

// skip if the parent is not a link
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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 This PR is ready for review. 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
Copy Markdown
Contributor

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

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. [Pri] BLOCKER

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants