Conversation
|
Size Change: +3.14 kB (+0.11%) Total Size: 2.99 MB
ℹ️ View Unchanged
|
30 tasks
madhusudhand
force-pushed
the
gallery/lightbox
branch
3 times, most recently
from
5af6a6b to
4fdbd86
Compare
Member
Author
|
@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
Contributor
|
This PR should close #37652. |
Contributor
|
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? |
Contributor
|
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! |
Contributor
|
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? |
Contributor
|
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
Member
Author
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. |
Member
Author
|
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. |
Contributor
|
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? |
Contributor
|
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. |
Contributor
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 😅 |
Contributor
I think ideally these are |
Contributor
I tried to implement this.
0a0255f4ad2455af7b78250269be1ff1.mp4 |
Contributor
|
What's are wiggleroom to iterate this post-merge? If there is some, I think we should consider landing this soon. A question, though: can we use the existing built-in breakpoitns for mobile and tablet? I recognize your numbers are nicer, but they are new values that are introduced here, but if we get access to defining and customizing the breakpoints in theme.json as discussed here, then it would presumably also work here. I think that's the main thing to change: one set of breakpoints for everything on the frontend. |
|
As always, great progress @t-hamano !
I think I'm with @jasmussen here. The The x is currently aligned to the x-height. Can we align it to the Cap height? I think this would look more even. The same goes with the Previous and Next arrows, even though it's not that visible because the arrows have the nearly same height as the capitals. 
|
Contributor
I fixed it, and it should line up correctly now.
Is this the intended behavior? I might be misunderstanding 518ba952507155eb41f40ab433db9ba7.mp4 |
Contributor
|
I think that's right, though if I'm reading Hannes correctly, he's suggesting "Close [X]" instead of "[X] Close". I don't personally have an opinion on that, but perhaps a followup question: text labels are off by default, yes? (That would be my expectation) |
yes, that's what i meant. Thanks!
I didn't think about that, but now that you mention it, it's not a bad idea! It'd be make the layout even more harmonized and the X would have a similar (not same) position as for navigation overlays, which (I think) makes the UX better for website visitors. I quickly tested in in playground and changed the order via the inspector. I think it's working great both on desktop and mobile. 
yes, at least when testing on playground |
Contributor
|
Update: I improved the UI for changing the navigation button type.
This should be consistent with the UI for the Navigation Overlay Close block: 
I'm not sure we should make this change in this pull request. In the Navigation Overlay Close block, 
Probably, can we discuss this point as a follow-up? This is because the change also affects images with Lightbox enabled outside of Gallery blocks. |
yes, good idea. |
Contributor
|
Thank you all for your feedback. If there are no other critical issues, I plan to merge this PR in time for WordPress 7.0. |
Contributor
|
I think it's worth landing. I would also love to see swipe-to-navigate on mobile, which is not a blocker for this PR, and perhaps not even feasible for 7.0. But I mention it in case a) it's trivial to add as a followup, and b) to ensure that it's not impossible to add in a followup due to the architecture. I'll trust you as developers on this. |
Contributor
@jasmussen I think this is already implemented 😄 0e041e344091645076d6766145565da6.1.mp4 |
Contributor
|
:whoah-emoji: Nice. Some visual sliding feedback would be a nice enhancement, so you can see yourself actually moving the images. But this is solid, thank you! |
jasmussen
requested review from
jasmussen
and removed request for
geriux and
jasmussen
Contributor
|
Apologies for the request changes: I meant to review/approve this one, but looks like it's already green. |
Contributor
|
Thank you everyone for sticking with this PR for so long. I'd like to merge this PR, but if there are any issues in the future, I will fix them before the official release of 7.0. |
github-project-automation
bot
moved this from 🔎 Needs Review
to ✅ Done
in WordPress 7.0 Editor Tasks
MaggieCabrera
pushed a commit
that referenced
this pull request
Feb 10, 2026
* add lightbox support to gallery block * disable and hide next/prev icons based on device * remove view.js from gallery * fix a lint issue * rename isGallery to hasNavigation and imageInit method * adjust button opacity when disabled * address accessiability of lightbox navigation * fix accessibility: dynamic aria-label based on current image * Try to fix a11y issues * Improve screen reader reading * move translations to server * reset gallery_id after each gallery processing * fix php lint errors * Use block context to pass down a gallery ID * Fix frontend implementation * Fix Screen reader double reading * Don't hide lightbox when hitting Enter on prev/next buttons * Match visual order with DOM order * Do not apply opacity to focus styles on disabled prev/next buttons * Add a tooltip to navigation button * Add `cursor:pointer` to navigation buttons * Add focus trap * Add "Lightbox navigation" option * Restore aria-live/aria-atomic * Ajust z-index values * Regenerate fixtures * Fix PHP lint error * Show toggle only when lightbox.allowEditing is enabled * Fix: add data-wp-watch--inert * Remove tooltip from previous and next buttons * Enable Lightbox by default * Add some reset CSS to the navigation buttons so they aren't affected by theme styles * Fix since tag * Add control to show visible text fo three buttons * Enhance text of Enlarge button * Regenerate fixtures * Make navigation button appearance more configurable * Update fixtures * Remove aria-label attirbute if value is empty * Update help text * Simplify logic * Fix: navigationButtonType doesn't work properly when there are multiple galleries * Output aria-label only when there is no visible text * Reduce icon size when button has text * Loop through the gallery * Disable button type selection UI when there is no lightbox image * Add mobile swipe action * Tweak lightbox button style * Apply randomize order setting to lightbox images * Fix unused state * Update @SInCE docblock to 7.0.0 * Use sentence case in label * Adjust size of close and navigation buttons * Remove navigation button wrapper div * Make navigation button responsive * Aligning icons and text * Change navigation button position * Use ToggleGroupControl for navigation button type control --------- Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Luis Herranz <[email protected]>
Member
|
Thank you @t-hamano for the sustained work on this one |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?
but can be disabled in the block sidebar. If the feature is disabled, the image lightboxes themselves will work as before, but navigation between images will not be enabled:Update: Changed to always be enabled.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
Disable "Lightbox navigation" in the Gallery block sidebar. A lightbox will open when you click on an image, but the back and next buttons will not be displayed.Testing Instructions for Keyboard
Tooltip text appears when the back and next buttons are hovered over or focused.Update: Based on this feedback, we removed the tooltip and instead added the aria-label attribute to the button.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