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

Multiple image blocks set to open in light box should connect to create cohesive carousel. #56310

Open
jeffgolenski opened this issue Nov 20, 2023 · 24 comments
Assignees
Labels
[Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement.

Comments

@jeffgolenski
Copy link

What problem does this address?

Currently, if I have several full sized images within a post set to open in a lightbox, it's very tedious to view the next image. Each image in a post must be clicked or tapped, then closed in order for viewer to get to the next one. It's not a fluid experience for the viewer.

If I have multiple images in a post set to open in the lightbox, a viewer should be able to tap on one to open the lightbox, and then be able to fuildly navigate to each image within the lightbox, like a carousel.

What is your proposed solution?

  • Desktop: User should be able to use left or right arrow keys to navigate through all the images in the same lightbox
  • Mobile: User should be able to swipe left or right to easily browse all images in the lightbox

Example: In my post I have many images set to open in the lightbox. If I tap on the first image, the lightbox opens, but to view the others I have to close it and tap the other images to re-open the lightbox.

IMG_2492
IMG_2493

cc @mtias @jasmussen

@jeffgolenski jeffgolenski added the [Type] Enhancement A suggestion for improvement. label Nov 20, 2023
@jeffgolenski
Copy link
Author

@jasmussen
Copy link
Contributor

Good issue, would definitely agree that if we are going to have lightbox enabled in galleries, it should either be a nice gallery there as well, chevrons left and right plus arrow key navigation. Or, just be disabled. I could swear there was already an issue for this, but I didn't find one linked from #51132. CC: @michalczaplinski

@michalczaplinski
Copy link
Contributor

I don't recall there being an issue for this yet, so thank you @jeffgolenski!

@jordesign jordesign added the [Block] Image Affects the Image Block label Nov 20, 2023
@michalczaplinski
Copy link
Contributor

michalczaplinski commented Nov 21, 2023

I've linked this issue in Tracking issue: Image Lightbox #51132

@bradhogan
Copy link

+1 on this - very common in all other platforms (webflow, sqs, etc.) would love to see this added soon. Thanks all.

@jacobgkau
Copy link

Just to clarify behavior, this seems desirable for multiple images within a gallery, but not necessarily desirable for all images in a post. E.g. if I have multiple galleries (which might be separated by paragraphs of text), I'd expect (or like the ability to configure) the lightbox to let someone flip through the images in a single gallery, but stop them when they get to the end of that gallery so they know to continue reading. This is how the baguetteBox.js lightbox plugin I've been using on my site operates.

@jacobgkau
Copy link

Related: #37652 (this one isn't linked on #51132 yet, maybe that's the one @jasmussen was thinking of earlier? It probably got lost since it was opened long before the 6.4 lightbox was implemented.)

@veerap000
Copy link

+1

@aileenf
Copy link

aileenf commented May 15, 2024

Another +1 here. A wee bit disappointed that there is no hint of this for 6.6. The single image lightbox works nicely... if gallery support is added, that would be a great thing. Thanks!

@jasmussen
Copy link
Contributor

I wanted to reference #56587 as having mockups relevant to this issue, notably this one:

lightbox

As far as a design for how this could look, at least the experience of it, IMO those designs can work.

As far as the block editor UI for it, I would recommend keeping things as simple as possible. Right now the way you set lightbox on individual images is in the link dialog:

Screenshot 2024-07-03 at 15 18 49

Choose "expand on click". The motivation for this is that it's a click behavior, just like visiting a link is, and opening a lightbox is mutually exclusive to zooming.

It's honestly the same for galleries and gallery lightboxes, so would a good meaningful first step be to add "Expand on click", and the improved dropdown appearance (which features icons) to this dropdown?

Screenshot 2024-07-03 at 15 19 05

It does open a question we have to answer eventually: what happens in a 5 image gallery if all but the third image is set to "expand on click"? My instinct: lightbox arrowkeys would navigate images 1 and 2, then you'd exit, and then you could again swap between 4 and 5, then exit. Of course not ideal, but nevertheless reflecting what you can do.

CC: @mikachan

@luisherranz
Copy link
Member

what happens in a 5 image gallery if all but the third image is set to "expand on click"? My instinct: lightbox arrowkeys would navigate images 1 and 2, then you'd exit, and then you could again swap between 4 and 5, then exit. Of course not ideal, but nevertheless reflecting what you can do.

Can't we remove/hide the "Expand on click" option of the Image block when the image is inside of a Gallery block, and add a global "Expand on click" option to the Gallery block itself?

@jasmussen
Copy link
Contributor

Can't we remove/hide the "Expand on click" option of the Image block when the image is inside of a Gallery block, and add a global "Expand on click" option to the Gallery block itself?

It veers into exotic use-cases that, if we have to handle them all, might end up adding a lot of complexity.

  • Individual images in a gallery still need to be possible to link to arbitrary URLs.
  • What if you have three images, two of which are set to expand on click, then you multi-select all of them and transform into a gallery; would we need to change their markup in that same transform action?
  • What if you, using the list view, move an image block from one gallery to another?

The above is one of the reasons the gallery block currently has a dropdown for applying properties across all child blocks, and then fires a snackbar when it's done so. E.g. link to and resolution. These aren't properties of the gallery block itself.

Simply to get things moving, if it was possible to simply detect if more than one block in a sequence are set to open in the same lightbox, we'd avoid the complexity of handling the above edge-cases, and would then be able to evaluate whether we need to do more in the future.

@luisherranz
Copy link
Member

I don't object to the idea of starting simple to see how it works and then we'll see from there 🙂

But for the record:

Individual images in a gallery still need to be possible to link to arbitrary URLs.

This is fine, it could keep working just as it does now.

What if you have three images, two of which are set to expand on click, then you multi-select all of them and transform into a gallery; would we need to change their markup in that same transform action?

I don't think anything needs to be changed. "Expand on click" is just one attribute and nothing is added to the markup. All the Interactivity API directives are added dynamically in PHP. I don't think there's any problem with preserving that attribute as it is, and ignoring it when necessary.

So, simply, if the Image is inside a Gallery block, the Image block rendering would ignore its own attribute and consider the global "Expand on click" Gallery attribute instead.

Something like this (not real variable names):

$gallery_expand = $block['context']['galllery_expand_on_click'];
$image_expand   = $attributes['expand_on_click'];

if ( $image_expand && ! $gallery_expand ) {
  // If the Image is not inside a gallery block and has expand on click...
} elseif ( $gallery_expand ) {
  // If the Image is inside a gallery which has expand on click...
}

What if you, using the list view, move an image block from one gallery to another?

That would still work fine because the Image is not modified at all.

Actually, if you have one Image with "Expand on click" and you move it in and out of a Gallery block, it will preserve its "Expand on click" attribute when it's moved outside.


The only thing that would need to happen in the Editor UI is that the "Expand on click" button is hidden when the Image block is inside a Gallery.

Does that make sense?

Again, not opposed to starting simple 🙂

@jasmussen
Copy link
Contributor

Could work. But would that mean if I have a single image inside the gallery that links to https://wordpress.org, but the gallery itself is set to expand on click, then that hyperlink is simply ignored, but still in the DOM? This is probably the main headache, as screen readers would say one thing, but the click behavior would say another. Let me know if I misunderstood.

@luisherranz
Copy link
Member

We can delete the hyperlink dynamically in PHP during the Image rendering if the $gallery_expand attribute is set to true, so that the HTML sent to the browser is correct but the hyperlink is still there if you move the Image block outside of the Gallery block.

We are entering the WordPress' HTML API era where things that were really difficult before are about to become trivial 😄

@jasmussen
Copy link
Contributor

Gotcha.

From my current vantage point, it sounds like a better user-experience. It also sound like it may still be best to do in two steps; no smarts for first implementation, then the gallery-wide toggle as a followup step. But that's purely an instinct, and ultimately I'll defer to a developer on what is simplest to implement! Thanks for chatting through this.

@luisherranz
Copy link
Member

no smarts for first implementation, then the gallery-wide toggle as a followup step

That makes sense. Thanks Joen!

@paaljoachim
Copy link
Contributor

paaljoachim commented Jul 25, 2024

There are now atleast 3 few issues/PR's that are associated with each other.
Gallery: Add lightbox support
#62906 (PR)

Move gallery link controls to the block toolbar
#62762 (PR)

and this issue to have multiple blocks set to open in a lightbox that should connect to create a cohesive carousel.

We really need to break this down into smaller steps.
From how I see it.

  1. First adding the Expand on click option to the LINK To setting in the Gallery sidebar settings. At the same time looking at various scenarious that can impact this setting. Such as @t-hamano comment here:
    Gallery: Add lightbox support #62906 (comment)
    as well as the discussion above here. After various concerns are addressed then one can easier move on to in the various issues/PR's.

@artemiomorales
Copy link
Contributor

Thanks to all who have offered their thoughts and to @akasunil and @madhusudhand for starting work on this!

As suggested by @paaljoachim, I do think we should take a step back and consider the various possible scenarios of gallery support for the lightbox in combination with individual image settings and the configurations in theme.json.

@t-hamano has made a key observation in this issue:

However, we can't simply add the "Expand on click" option to the Gallery block, because we need to check whether the theme supports lightbox. And the current Image block has complex logic to decide whether to show this setting or not. Try searching for "lightbox" in the packages/block-library/src/image/image.js file.

Perhaps the Gallery block needs equivalent logic, and I think it will be necessary to thoroughly verify it in a separate PR to make it happen.

Here I'll go over the scenarios as far as I can see (anyone feel free to elaborate or correct anything).

Possible Lightbox Configurations

First I'll review some background just to make sure we're on the same page.

Many theme and plugin authors have their own lightbox implementations that conflict with the lightbox in core — they'll want to disable the lightbox and hide the UI so as not to confuse their end users.

In other cases, site builders who want to use the core lightbox may choose to enable it, yet still hide the UI to streamline the editing experience. Yet other users may prefer to have the lightbox enabled by default, but show the UI so they can disable it on a case by case basis.

For that reason, one is currently allowed to define lightbox settings for the image as follows in theme.jsonallowEditing controls whether the UI appears, and enabled determines whether the lightbox is enabled by default:

{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {
		"blocks": {
			"core/image": {
				"lightbox": {
					"allowEditing": true | false,
					"enabled": true | false
				}
			}
		}
	}
}

So the possible scenarios for the lightbox functionality are:

  1. The UI enabled, with lightbox turned on by default
  2. The UI enabled, with lightbox turned off by default
  3. The UI disabled, with lightbox turned on by default
    • (In this scenario, users cannot turn off the lightbox setting)
  4. The UI disabled, with lightbox turned off by default
    • (In this scenario, the lightbox is completely disabled)

How the Gallery Relates

For each scenario then, we should consider how the gallery's Expand on click setting relates. For now, I'll just go over the simplest approach outlined by @jasmussen above — adding the Expand on click option to the Link to dropdown.

Scenarios 1, 2, and 3 are fairly straightforward — we can simply respect the individual image settings, and if more than one image has the lightbox enabled, we then show the chevrons to allow flipping from one image to the other.

That just leaves scenario 4. The intention here is to disable the lightbox entirely, which means that I believe it would also make sense in this scenario to hide the Expand on click option from the gallery dropdown.

There are also edge cases to consider. What if a user enables Expand on click for a gallery using one theme, then changes their theme to one that has the lightbox UI and functionality disabled? Do we respect the original setting, or simply remember it in case they later switch to a theme that uses the core lightbox again?

There may be other scenarios, particularly as we explore the gallery lightbox settings potentially overriding the image settings.

Conclusion

Anyway, I hope this can help bring some clarity and alignment to the discussion and begin to unearth some of the potential edge cases.

For anyone interested in learning more, there are a few lightbox edge cases regarding image block-level overrides and how they interact with theme.json covered in #59890. These may have an effect on the gallery or offer food for thought on scenarios to consider depending on the approach we end up taking.

Just let me know if anything is unclear! I'm also happy to elaborate further or answer any questions on the above 🙏

@madhusudhand
Copy link
Contributor

@artemiomorales Thank you for the detailed explanation. ❤️

That just leaves scenario 4. The intention here is to disable the lightbox entirely, which means that I believe it would also make sense in this scenario to hide the Expand on click option from the gallery dropdown.

I Agree. Gallery Expand on click should also be controlled by the theme.json setting when it is being introduced as part of #62762 @akasunil It should be similar to how image currently behaves.

There are also edge cases to consider. What if a user enables Expand on click for a gallery using one theme, then changes their theme to one that has the lightbox UI and functionality disabled? Do we respect the original setting, or simply remember it in case they later switch to a theme that uses the core lightbox again?

Since user settings override, theme.json, switching a theme in this case would still keep the lightbox functionality, and I think it is expected to be preserved. Following is how an image behaves.

  1. Enable lightbox on an image (in a theme that has UI) and save the template.
  2. Now switch to a theme without allowEditing or go to theme.json of the current theme and turn it off.
  3. Observe that UI is still visible because the image has it in the template.
image

Once disabled, it is no longer available because of the new theme setting.

image

A gallery should have the option with same behaviour.

What do you think?

@akasunil
Copy link
Member

Gallery Expand on click should also be controlled by the theme.json setting when it is being introduced as part of #62762 @akasunil It should be similar to how image currently behaves.

I agree, we might have to follow the same logic that image block have for light-box, I'm planning to create a separate PR for this, which would give us better idea of use-cases where it fails.

@akasunil
Copy link
Member

Thank you @artemiomorales I appreciate your thorough explanation.

@artemiomorales
Copy link
Contributor

artemiomorales commented Jul 31, 2024

A gallery should have the option with same behaviour.

What do you think?

That sounds good to me.

Something does tell me that adding the lightbox to galleries will increase the potential for conflicts with existing 3rd party lightbox solutions, namely when folks decide to override the gallery lightbox using a block-level setting.

In any case, we can keep an eye out and determine later if a different behavior would be more appropriate.

@akasunil
Copy link
Member

akasunil commented Aug 1, 2024

A gallery should have the option with same behaviour.

What do you think?

I have opened this PR #64014 to implement lightbox setting on gallery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement.
Projects
Status: Next
Development

No branches or pull requests