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

Fix gallery thumbnails not opening a lightbox #20249

Closed

Conversation

mikeyarce
Copy link
Contributor

This commit introduced a bug which prevents gallery thumbnails from opening up in a lightbox: ffbbf08#diff-70e6d5d85f9bbb3126dad3190f865c2f

The commit message is a bit vague but it sounds like the main change was just adding e.preventDefault(); and then there was an else statement added after but not sure why. That else statement is not really needed because it’s not really doing an else, it’s just activating photoswipe for images after that if statement runs.

Removing the else brings back the lightbox for the gallery thumbnails.

This commit introduced a bug which prevents gallery thumbnails from opening up in a lightbox: woocommerce@ffbbf08#diff-70e6d5d85f9bbb3126dad3190f865c2f

The commit message is a bit vague but it sounds like the main change was just adding `e.preventDefault();` and then there was an `else` statement added after but not sure why. That else statement is not really needed because it’s not really doing an else, it’s just activating photoswipe for images after that if statement runs.

Removing the `else` brings back the lightbox for the gallery thumbnails.
@kloon kloon added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label May 28, 2018
@kloon
Copy link
Member

kloon commented May 28, 2018

@mikeyarce I am testing this on the current master branch and when I disable zoom functionality via add_filter( 'woocommerce_single_product_zoom_enabled', '__return_false' ); it disables it correctly and then still opens up the images in the lightbox.

If you look further atop in that function you will see it already attaches a trigger to the trigger element which is the magnifying class

this.$target.on( 'click', '.woocommerce-product-gallery__trigger', this.openPhotoswipe );

The gallery images should not open the lightbox when clicked but just load in the main image container, from there you can open them in the lightbox and scroll further.

Which theme have you tested this with?

@mikeyarce
Copy link
Contributor Author

@kloon I see what you mean. I think the problem is that when the Slider is disabled remove_theme_support( 'wc-product-gallery-slider' ); , then the images below should open up in a lightbox.

Here's Twenty Seventeen with the slider disabled:
lightbox

Sorry if I didn't specify that! This PR does make sure that this works when the Slider is disabled and opens up the lightbox in that scenario.

@mikejolley
Copy link
Member

This might regress the changes made for mobile support so we can just enable conditionally if the slider is disabled.

@mikejolley
Copy link
Member

Fix here #20290

Thanks

@mikejolley mikejolley closed this May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: author feedback The issue/PR needs a response from any of the parties involved in the issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants