Image: Adopt margin block support #63546
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
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
artemiomorales,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Flaky tests detected in 8d3e882. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9984889086
|
|
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 changes
Jul 15, 2024
There was a problem hiding this comment.
I tested adding a margin to an image aligned to the right with the lightbox enabled, and the margin causes the lightbox to not animate smoothly:
lightbox-margin.mp4
The added margin is likely throwing off the lightbox calculations; likely we should aim to take those into account.
|
The animation is also affected with center-aligned images that have a margin enabled: lightbox-margin2.mp4We should also take into account using different aspect ratios, custom sizing, different resolutions, both scale options 
Regarding border, I see the lightbox animation is actually broken on lightbox-margin3.mp4My guess is the setOverlayStyles callback would need to modified to make the margin support compatible with the lightbox. |
2 tasks
|
Thanks for reviewing @artemiomorales 👍 I honestly missed testing image lightbox functionality when adding the margin support here 😅 From what you've outlined though it sounds like there is a more general problem with lightbox not taking into account image styles that affect spacing such as margin and border, than a problem with the application of margins on an image. It is worth noting that you can already define margins for images via theme.json. Does this not also cause a problem or is the issue that block support styles are applied as inline styles and override lightbox resets etc? Do you think this issue needs to block the adoption of margin support for all images? Can it be addressed separately via |
@aaronrobertshaw It appears that the lightbox works as expected when defining the margin via theme.json or in the global styles — the issue, as you've pointed out, is that the styles are applied inline at the block level, notably to the parent
I think so, otherwise anyone who adds a margin to an image with Would it be alright if I worked on that this week as part of this PR? I'm happy to hear other ideas too, let me know what you think 🙏 |
|
Size Change: +2.16 kB (+0.12%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
artemiomorales
approved these changes
Jul 16, 2024
There was a problem hiding this comment.
@aaronrobertshaw It turns out that the lightbox copies the figure's inline styles in the overlay, and the new margins were throwing off the calculations. I pushed a commit to remove the margins from the overlay, and it seems to work properly now.
I can do another smoke test tomorrow but will go ahead and approve so you can merge if you're feeling confident with all of the use cases 🙏
|
@artemiomorales are there unit tests for the lightbox behaviour that can be updated to protect against regressions? |
@aaronrobertshaw There were but they need to be redone. I can take a look at that tomorrow, as part of this PR or a separate one, whichever you prefer. |
|
@aaronrobertshaw Ok I added a test to ensure the margin inline styles don't get applied to the overlay image 👍 |
|
Really appreciate the help getting the lightbox behaviour sorted @artemiomorales, thanks 👍 I've given this PR a run both before and after the latest updates. The animation looks nice and smooth again. Screen.Recording.2024-07-18.at.3.18.06.PM.mp4 |
andrewserong
approved these changes
Jul 18, 2024
There was a problem hiding this comment.
I'm not very familiar with the lightbox code and it took me a little while to find the control for it (is the only place it's exposed in global styles > blocks > image?). But it's working nicely!
✅ Image margin set in global styles is output correctly in post and site editors and on the site frontend
✅ Block level instance overrides work correctly
✅ In manual testing the lightbox feature still appears to be working as expected AFAIK
LGTM! 🚀
|
Thanks for the review @andrewserong 👍
There is also the link block toolbar button. Under that you'll have the option to choose "Expand on click". I agree though, the discoverability of it isn't fantastic. It took me a while and I knew it was there and was actively looking for it. |
Thank you! That was a TIL for me, I didn't realise it was under there, I was looking at the inspector sidebar. Cheers! |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: artemiomorales <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: andrewserong <[email protected]>
2 tasks
97 tasks
7 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.
Part of: #43241
What?
Why?
How?
Testing Instructions
Note: Global margins will be overridden by layout whereas block instance margins will not be.
Screenshots or screencast
Screen.Recording.2024-07-15.at.1.14.56.PM.mp4