Gallery: Add lightbox support #62906
Conversation
|
Size Change: +882 B (+0.05%) Total Size: 1.83 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 |
|
Several options have been proposed in this GitHub discussion for generating all possible translated strings on the server when a lightbox is enabled for the gallery block. However, these options are complex, making it difficult to decide which approach to take, especially given the limitations due to the inability to use the well-established JavaScript i18n support introduced in WordPress 5.0. If we were using regular scripts, the following client-side code would have sufficed: import { __, _x, _n, _nx } '@wordpress/i18n';
__( '__', 'my-domain' );
_x( '_x', '_x_context', 'my-domain' );
_n( '_n_single', '_n_plural', number, 'my-domain' );
_nx( '_nx_single', '_nx_plural', number, '_nx_context', 'my-domain' );For more complex cases where variables need to be used, the However, before we can bring this same native experience to ES Modules, we must resolve the ongoing issue of compatibility between scripts and script modules. This will pave the way for better integration with the translation pipeline, which is already being tracked in Script Modules API: Add a translations API. This presents us with a significant dilemma. Do we continue working under the assumption that the limitations mentioned above are acceptable, hoping that we can iterate quickly by finding suitable workarounds for every edge case? Or is it time to step back and resolve the root issue, ensuring that developers using the Interactivity API can have the same seamless experience they've enjoyed for years—handling translations both server-side in PHP and client-side in JS using the i18n helpers that have been a core part of WordPress? This same principle applies to other existing APIs; for instance, developers familiar with In conclusion, I hope that the experience gained while improving the Gallery block will encourage us to reassess our priorities. We should aim to avoid situations where we can’t use tools that WordPress developers have relied on for years, and where we too quickly concede that something isn't supported with ES Modules. If we agree that user experience should take precedence over the technical solution, then I urge you to consider the following aspects when deciding on the final approach:
|
|
I'll spend some time looking into what's involved for providing functionality from the accessibility packages as a script module. It doesn't depend on That can be translated on the server and passed to the module via script module data. This provides a good opportunity to explore options around modules built from existing packages. I have some exploration in #60952 that can serve as inspiration. |
madhusudhand
force-pushed
the
gallery/lightbox
branch
from
6b233eb to
3716092
Compare
|
Love seeing the a11y module integration exploration. This will be super helpful. Thanks! |
|
Looks like more work is needed here so marking this as punted to 6.8. |
I respectfully disagree 😄 From the options I outlined above, the only thing that makes things complex is the inability to share a simple PHP function between two Gutenberg blocks due to the complexities of the build system and the automatic renaming of the Gutenberg <-> Core functions. If that weren't the case, we wouldn't be having the conversation about the JavaScript i18n module or about any of the alternative solutions. Solving this problem with JavaScript requires sending an additional 5 kilobytes to the front end, plus loading that code and processing the strings. In contrast, sending those translated strings directly from the server only requires the bytes that those strings occupy, with no additional download and processing needed.
Yes, I believe that user experience should take precedence over the technical solution. We should also set an example in core blocks: if something can be translated on the server, it should be translated on the server. The external plugin blocks don't have our limitations when it comes to sharing functions between two Gutenberg blocks, so that should be the solution we teach them for this pattern. That doesn't mean we don't need to create a module for the i18n package. It simply means that the package should be used exclusively in exceptional cases where server translations cannot be used. In my humble opinion, this is not one of those. EDIT: Passing down block context would also be a simple and acceptable solution, and actually even better than the shared function as it would pass down the information in the context of the block structure, but only as long as it doesn't require the creation of a serialized attribute (I'm unsure it can work that way at this moment). |
| @@ -355,6 +438,14 @@ const { state, actions, callbacks } = store( | |||
| } | |||
| `; | |||
| }, | |||
| setScreenReaderText() { | |||
| const { ref } = getElement(); | |||
There was a problem hiding this comment.
It is now possible to use @wordpress/a11y module and speak method. See the dev note from @sirreal: https://make.wordpress.org/core/2024/10/14/updates-to-script-modules-in-6-7/.
Example usage in @wordpress/interactivity-router for reference:
gutenberg/packages/interactivity-router/src/index.ts
Lines 421 to 425 in b7d989e
Remember that the processing still happens on the server for every possible image that can be enlarged. That is necessary with the proposed approach even when the user never opens the lightbox, which is the prerequisite to use the translation.
@sirreal had these justified idea that |
|
As we still don't have a module I've tried using the block context to pass a gallery ID with the Right now, the frontend implementation is broken, and I need to adjust it to this change, but I'll do it next Monday. |
There was no progress on that i18n front in the last three months. I guess the server approach is the only viable option that will allow to move it forward so we can start testing it with users 👍 |
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