Gallery: Add lightbox support #62906
Conversation
|
Size Change: +1.44 kB (+0.08%) Total Size: 1.85 MB
ℹ️ View Unchanged
|
madhusudhand
force-pushed
the
gallery/lightbox
branch
3 times, most recently
from
5af6a6b to
4fdbd86
Compare
|
@jasmussen @WordPress/gutenberg-design This is a first iteration of #56310.
The same design doesn't work for smaller devices as the image fits to 100% width. 
While icons can be avoided in touch based devices, that may not be accessible. |
t-hamano
added
[Type] Enhancement
|
This PR should close #37652. |
|
The chevrons look a bit awkward when they get this close to the image: 
Ideally there should be no overlap. When viewing the first or last image in the lightbox, the next / previous button is disabled accordingly, but there's no visual indication which makes the button feel broken. Two options to explore:
I'm not sure the chevrons need to appear on mobile, swiping left/right is probably more intuitive? |
|
Great! Thank you for working on this @madhusudhand Taking a step back. Doing some brain storming. EDIT:
Add a toggle to add left/right pagination for the Expand option. So it would be something like this. Thanks! |
|
In #62762 we are moving the Link to option from the block sidebar to the block toolbar. If we make this change, will it conflict with this PR? |
|
I think we need to consider various scenarios. Here are some examples:
|
madhusudhand
force-pushed
the
gallery/lightbox
branch
2 times, most recently
from
a8ca784 to
bd0700e
Compare
As per the current changes, lightbox invoked by clicking on any image except 3, and next/prev navigation moves the lightbox images between 1,2,4,5 without exiting after image 2 (on next). I would imagine this would be better experience have lightbox as single set [1,2,4,5], instead of breaking into two sets [1,2] and [3,4]. Please suggest.
@t-hamano This PR now only based on the image having "Expand to click" enabled. I don't think it will conflict. The behaviour should be same as long as the image gets the value for lightbox.
This will be
They will be skipped and it is the right thing to do, because clicking on those images directly doesn't invoke lightbox.
This is not relevant for this iteration. But #62762 makes it possible, when the setting is enabled at a gallery block level. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Images are given a better padding to avoid overlaps with the icon.
Yes. The current change disables the buttons. Suggest on opacity.
They are removed on mobile devices, meaning the current PR has no effect on mobile devices and swipe actions will be added in a followup PR. |
|
Thanks for the detailed explanation. Since it's based only on whether the Gallery block has images with lightbox enabled, does that mean there's no way to disable navigation between images? Do we need to provide some way to disable (or enable) navigation between images? |
|
The Link to option in the Gallery block is just a global change to the With that in mind, I think there might need to be an option to explicitly enable the Gallery lightbox. For example, the UI would look like this:
However, if we move the block sidebar's "Link to" control to the block toolbar in #62762, the question is whether to leave the "Enable lightbox Gallery" toggle. |
To be honest, I don't see many cases where people would want to disable image navigation. It just bothers me that there's no way to opt out of it 😅 |
I think ideally these are |
luisherranz
force-pushed
the
gallery/lightbox
branch
from
9b58c81 to
fd51250
Compare
Ok, this is fixed now. Sorry again for taking so long. |
|
@luisherranz Thank you for your response! I've made some changes based on the feedback we've received so far, primarily the accessibility feedback in the comment here. There are still some things left to be addressed and I'm not sure yet if we'll be able to ship this PR into 6.8, but I think it's ready for basic testing.
I applied the opacity to the svg element inside, rather than to the button itself, so that disabled buttons don't get a faded outline when focused:
We could use the
By moving the next button after the image, the DOM order matches the visual order. |
I haven't done anything about this, but maybe fd51250 has improved it so that aria-label is localizable? |
t-hamano
requested review from
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Ok, all feedback should have been addressed. See here for the latest summary and testing instructions: #62906 (comment) |
Yes, all translations are now done on the server so there's no need for a frontend i18n package. |
|
@WordPress/gutenberg-core I would like to discuss whether this PR can/should be shipped to 6.8. This PR enables navigation between lightbox images in the Gallery block. This PR wasn't ready in time for 6.7 so it has been punted to 6.8. For the latest description, see the first comment for details. For accessibility feedback: @afercia @joedolson @alexstine |
|
I'm concerned about this implementation and I think there's a few things that should be carefully considered before merging and releasing a feature that risks to be changed significantly in following releases, thus introducing backwards compatibility issues. Pinging @joedolson and @felixarntz for more context on the ongoing discussion on #55513 From an a11y perspective:1 2 3 General considerations4 5 6 |
There was a problem hiding this comment.
Based on the comment at #62906 (comment) I'm not sure this PR should be in an approved state yet.
While the effort here is remarkable and very welcome, to me there's still many things to be considered. I'm not willing to block this PR but I do think more feedback and more eyes are necessary before marking this PR as approved.
|
Thank you for your great feedback! It seems that there are still many issues to be addressed in this PR. In particular, the following point is the most important and also the most technically difficult:
Given this, I think it will be difficult to ship this in 6.8. I propose to punt this PR until 6.9 or later. |
|
Agreed. Let's move this to WP 6.9. |
Originally written by @madhusudhand
Details
What?
This introduces lightbox functionality to
Galleryblock.How?
Following is the behaviour in various scenarios.
Case 1: "Expand on click" is enabled for the image block globally.
All images inside (unless overridden at block level), will invoke the lightbox on click and next and previous navigation is possible.
Case 2: One of the image (lets say 2nd image) is set to "Link to image file"
Lightbox will be invoked on click of 1st image and on click of next will move to 3rd image.
Case 3: "Expand on click" is not enabled globally, but few images are set with this option in gallery.
This is similar to case 2, lightbox opens with only those images.
What's in future PRs?
Screenshots (Draft)
Testing instructions
Related issues
First iteration of #56310
Latest description written by @t-hamano
What?
This PR enables navigation between images with lightbox enabled.
Why?
#56310
How?
aria-label, inject an element witharia-liveandaria-atomicattributes and vary the text inside. We could also usespeak, but the@wordpress/a11yscript module is available from WP 6.7. The Gutenberg plugin has to support WP 6.6 for now, so I didn't go with the speak approach.Testing Instructions
Testing Instructions for Keyboard
Enlarged image X of Y: {IMAGE_ALT_TEXT} dialogbutton unavailable Previous,button NextEnlarged image X of Y: {IMAGE_ALT_TEXT}Screenshots or screencast
6999948663987cac5545ba0e9eaa1a88.mp4