Image lightbox: move image data from context to state #63348
Merged
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
|
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. |
madhusudhand
commented
Jul 10, 2024
madhusudhand
added
[Type] Code Quality
madhusudhand
force-pushed
the
image-lightbox-refactor-context
branch
from
a9d9274 to
4085ad2
Compare
|
Size Change: +2.87 kB (+0.16%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
mikachan
reviewed
Jul 10, 2024
There was a problem hiding this comment.
This is more of a smoke-test result, but I gave this a test and I can see the lightbox is behaving the same for both the Image block and the Gallery block. I can't see any errors or issues 🙌
Looks like there's a small coding standards issue in image/index.php, but other than that, the code reads well to me 👍 I can see the changes to data-wp-context and wp_interactivity_state are moving the image data from context to state, which looks good to me. With the caveat that I'm not too familiar with wp_interactivity_state yet!
madhusudhand
force-pushed
the
image-lightbox-refactor-context
branch
from
4085ad2 to
6972c58
Compare
madhusudhand
force-pushed
the
image-lightbox-refactor-context
branch
from
7aea9b1 to
a24fa62
Compare
luisherranz
previously requested changes
Jul 12, 2024
|
@SantosGuillamot could you please test this with your image set? If everything works, feel free to dismiss my review and approve the pull request 🙂 |
|
Thanks for the review and changes @luisherranz |
SantosGuillamot
requested changes
Jul 13, 2024
There was a problem hiding this comment.
The UX seems broken in my local tests:
In this pull request
Lightbox.PR.mp4
In trunk
Lightbox.trunk.mp4
After taking a quick look, it seems it breaks after this commit: Use hash instead of counter for the unique id. If I revert that changes, all the images seem to work as expected.
Since multiple images can have same image source URL, same has is generated for all those images and so the image context is getting overridden.
|
|
There are already some uses of |
|
@SantosGuillamot could you take another look? Thanks! |
madhusudhand
commented
Jul 15, 2024
Comment on lines
+409
to
+410
| state.metadata[ imageId ].imageButtonTop = imageButtonTop; | ||
| state.metadata[ imageId ].imageButtonRight = imageButtonRight; |
There was a problem hiding this comment.
@luisherranz we might have to revert to keep the data in context instead, because it is being used in the template, not anywhere else.
It is breaking the icon placement.
There was a problem hiding this comment.
I can reproduce this as well.
There was a problem hiding this comment.
We can use derived state, similar to how we are using in other places:
store('...', {
state: {
get imageButtonRight() {
const { imageId } = getContext();
return state.metadata[imageId].imageButtonRight;
},
get imageButtonTop() ...
},
});<button
data-wp-style--right="state.imageButtonRight"
data-wp-style--top="state.imageButtonTop"
>
...
</button>There was a problem hiding this comment.
Thanks for the suggestion. The issue is fixed now.
SantosGuillamot
approved these changes
Jul 15, 2024
There was a problem hiding this comment.
It looks good on my testing 🙂
EDIT: I didn't check the icon as reported here. It's true that part is broken.
|
Flaky tests detected in d0d8a85. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9936606436
|
luisherranz
approved these changes
Jul 15, 2024
There was a problem hiding this comment.
Ok, this should be ready now. Thanks, Madhu and Mario.
Let's merge it and continue working on the fix for the duplicate images and the Gallery block 🙂
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
* move the image context data to state * move image and button refs from context to state * Use hash instead of counter for the unique id * Don't use immutability * Fix reading prop from old context * Only store the current image ID in the state * Replace hash based on url with PHP's uniqid() * fix lightbox trigger image position --------- Co-authored-by: madhusudhand <madhudollu@git.wordpress.org> Co-authored-by: luisherranz <luisherranz@git.wordpress.org> Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
6 tasks
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 change moves the image data from context to state.
The data is set in the set against a
unique_idcreated for the image.After this change, image data of all images can be accessed directly from the 'core/image` state.
Why?
This helps to control the image lightbox from containing blocks such as
gallery.In a future change, gallery can will hold list of
image_idsin its context, and sets the image block with currentimage_idto show lightbox as a carousel.Gallery block cannot access context data of all images.
Alternatively, gallery could have an array and each image block may push its context data to gallery array
on image init, but it make the same data to be present in two places.Hence the image metadata is moved to its state.
Testing Instructions