Image lightbox: Remove duplicate image when lightbox is opened #63381
Conversation
This file contains 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
This comment was marked as outdated.
This comment was marked as outdated.
artemiomorales
added
the
[Type] Bug
|
Size Change: +1.01 kB (+0.06%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 40c28b9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9980916405
|
artemiomorales
commented
Jul 13, 2024
| @@ -191,6 +191,7 @@ function block_core_image_render_lightbox( $block_content, $block ) { | |||
| 'data-wp-context', | |||
| wp_json_encode( | |||
| array( | |||
| 'animationId' => uniqid(), | |||
There was a problem hiding this comment.
We need to set a unique value in the context to make sure we only hide the content image that is currently being clicked. Is uniqid() a good way to do that?
There was a problem hiding this comment.
As this is more like unique id of the image, better name would be imageId.
It is being introduced in this change https://github.com/WordPress/gutenberg/pull/63348/files#diff-1e4aa29ea183efda9c795271dc1acf6e8049355db104d0dca5b0ea5caf7f619cR218
There was a problem hiding this comment.
imageId is also an existing attribute, so I would recommend using something more tied to the behavior of the overlay that is getting animated.
|
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. |
artemiomorales
requested review from
madhusudhand,
gziolo,
luisherranz and
SantosGuillamot
madhusudhand
approved these changes
Jul 15, 2024
luisherranz
requested changes
Jul 15, 2024
|
The pull request that adds unique ids to the images has already been merged: @artemiomorales, can you refactor this pull request on top of the other and see if we can use the unique id we are already generating? |
artemiomorales
force-pushed
the
fix/lightbox-duplicate-image
branch
from
39fe590
to
72b3688
Compare
@luisherranz Done! |
SantosGuillamot
approved these changes
Jul 16, 2024
There was a problem hiding this comment.
It works good with image and gallery block.
Not directly related to this,
Observed a small issue with images that have a caption, in a gallery block.
There is a slight shadow left on the background.
Steps:
- Add a gallery block and set captions for few images.
- Set the scroll position of the page to a place where image bottom is near window bottom.
- Now click on the image (with caption)
- Observe that there is a shadow in place of original image.
Was this introduced in this pull request or did it exist before? |
luisherranz
approved these changes
Jul 17, 2024
artemiomorales
commented
Jul 17, 2024
There was a problem hiding this comment.
Was this introduced in this pull request or did it exist before?
@luisherranz @madhusudhand Previously, the caption faded out at the same rate as the duplicate image, so it wasn't an issue. Effectively, this pull request is introducing the problem.
I've updated the code, putting the class name on the parent figure instead. I've also highlighted part of the fix below, though I'm unsure if this is the best approach. Would appreciate any feedback 🙏
lightbox-gallery-caption-fix.mp4
Comment on lines
14
to
37
| @media (prefers-reduced-motion: no-preference) { | ||
| &.hide-content { | ||
| img { | ||
| visibility: hidden; | ||
| } | ||
| figcaption { | ||
| .wp-block-gallery & { | ||
| visibility: hidden; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &.show-content { | ||
| img { | ||
| animation: show-content-image 0.4s; | ||
| } | ||
| figcaption { | ||
| .wp-block-gallery & { | ||
| animation: show-content-image 0.4s; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm unsure if we should put the styles to hide/show the caption here or in the gallery styles. My inclination is to put them here because we need to animate the caption with the same timing as the image. What do you think?
Note that we need to restrict these caption styles to the gallery to prevent hiding captions in other circumstances when they don't overlay the image.
There was a problem hiding this comment.
What is the desired behavior with captions here? Should they be handled different for gallery vs normal images?
Right now this is how galleries are working in this pull request:
Lightbox.lightbox.-.18.July.2024.mp4
And this is how normal images work:
Lightbox.caption.1.mp4
This is how they would work if we hide the caption as in the galleries:
Lightbox.gallery.mp4
So, what is the expected UX? Should we even add a "fade-in" effect for the caption as well?
There was a problem hiding this comment.
It is a separate issue to handle the captions correctly. I'm also wondering what the rationale was for omitting the caption in the expanded image. There are reports that the caption should be included for legal reasons on some websites #60469 (comment). So that at least should be an option to configure for the lightbox.
There was a problem hiding this comment.
So, what is the expected UX? Should we even add a "fade-in" effect for the caption as well?
Ok got it, so we need to account for the caption in galleries in two scenarios:
- When the caption overlays the image
- When the caption does not overlay the image
I only took into account the first issue, not the second.
In the first scenario, like in the latest fix, I think we could make the caption disappear immediately when the lightbox opens, then make it reappear at the proper time when the lightbox is closed. We could also introduce a fade and sequence it properly with the zooming animation to make it more elegant.
For the second scenario, the caption should fade out and fade in smoothly like the rest of the content outside of the image.
I'm also wondering what the rationale was for omitting the caption in the expanded image. There are reports that the caption should be included for legal reasons on some websites #60469 (comment). So that at least should be an option to configure for the lightbox.
I believe there was conversation with design that the experience of a single image lightbox doesn't require a caption because one already has the context to understand what's being displayed. This is in contrast to a lightbox gallery, in which you don't have as much context.
It is a separate issue to handle the captions correctly.
Ok, I'll revert the most recent change, merge, and open a separate issue 👍
This reverts commit 40c28b9.
Open
2 tasks
|
@madhusudhand @gziolo @SantosGuillamot I opened a new issue to continue the discussion regarding the caption behavior. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…ress#63381) * Hide content image when lightbox is opened * Handle duplicate image when closing lightbox as well * Remove styling for prefers reduced motion * Rename state getters * Hide captions in galleries when opening lightbox * Revert "Hide captions in galleries when opening lightbox" This reverts commit 40c28b9.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
This PR hides the duplicate image that shows when the lightbox is opened, reported in #55960
Why?
It's a better user experience if the image is not duplicated.
How?
It sets the visibility to
hiddenon the content image when the lightbox is opened, and uses CSS keyframes to set the visibility back tovisibleat the appropriate time when the lightbox is closed.Testing Instructions
Expand on ClickPlease also test on mobile and in different browsers.
Screenshots or screencast
Before
lightbox-duplicate-image-bug.mp4
After
lightbox-duplicate-image-fix.mp4