Image Cropper: Add interactive grid lines state#77683
Open
andrewserong wants to merge 2 commits intotrunkfrom
Open
Image Cropper: Add interactive grid lines state#77683andrewserong wants to merge 2 commits intotrunkfrom
andrewserong wants to merge 2 commits intotrunkfrom
Conversation
andrewserong
added
[Feature] Media
|
Size Change: +451 B (+0.01%) Total Size: 7.77 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 8f2f176. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25037076295
|
ramonjd
reviewed
Apr 28, 2026
Member
There was a problem hiding this comment.
Thanks for getting this up. I like the idea of showing the grid for certain interactions.
Just to help me test, what's the intended UX? The grid is triggered on "state changed", from which we infer that the user is interacting. Did I understand that right?
It might be just me, but I think that's a bit too noisy. For example, the grid shows also for:
- flip (no other changes)
- hitting reset
- rotate (no other changes)
- aspect ratio change (no other changes)
I think the grid is most useful for composition- or placement-like operations, so where the left/right values change (crop + zoom + pan - fine rotate does these too).
So what I believe I'm trying to say is I wonder if this feature should be driven by interaction events, not only by cropper state changes.
Locally I tested something out. The main idea was to move grid show/fade into useInteraction. So:
- Add a flag (e.g.,
isPlacementInteracting) from useInteraction for pan/drag and keyboard arrow pan. - Treat crop-resize handles separately, including keyboard handle resizing.
- Keep a value-change fallback only for zoom/pan placement changes.
- Suppress grid flashes for flip, rotate, reset, and aspect-ratio changes.
- Shorten fade delay to 100ms.
- Let zoom use the short value-change flash instead of the longer wheel gesture debounce.
2026-04-28.09.50.08.mp4
This might be too big of a refactor for this feature - though. Is there a middle ground? 🤔
Sorry, I didn't want to derail an otherwise good idea!
| controller={ controller } | ||
| aspectRatio={ aspectRatio } | ||
| freeformCrop={ freeformCrop } | ||
| showGrid="interactive" |
| * @param root0.controller The full state/setter object from `useCropperState`. | ||
| * @param root0.stencil Custom stencil component. | ||
| * @param root0.showGrid Show rule-of-thirds grid overlay. | ||
| * @param root0.showGrid Grid overlay mode: false | true | 'interactive'. |
| const CROP_RECT_EPSILON = 1e-6; | ||
|
|
||
| /** How long to wait after the last interaction before fading the grid out. */ | ||
| const GRID_FADE_DELAY_MS = 800; |
Member
There was a problem hiding this comment.
This is a purely personal preference, so not a blocker or anything.
To me the grid lingers longer than necessary, and conflicts with the general snappiness of the thing.
2026-04-28.09.26.09.mp4
I don't know if this is a technical constraint of the state chain...
Contributor
Author
There was a problem hiding this comment.
Not a technical constraint, I kind of threw 800 at it to try to make sure if you're using the fine-grained rotation that it doesn't hide before you want it to. But right now (and after re-testing after sleeping on it), I agree, it feels a bit sluggish. I'll lower it 👍
Contributor
Author
There was a problem hiding this comment.
I'll lower it 👍
Edit: I read your comment here before your full review comment. If you've got something working locally, I'd say push that and we can iterate from there!
Comment on lines
+414
to
+416
| useEffect( () => { | ||
| return () => clearTimeout( gridFadeTimerRef.current ); | ||
| }, [] ); |
Member
There was a problem hiding this comment.
Is this required? It's cleaned up in the useEffect above. Or just a fallback case?
|
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. |
Contributor
Author
Thanks for testing @ramonjd! I think your instincts are right, and the more I think about it, I think it does need to be tied to the user interaction. I.e. if I hold down my mouse cursor while doing fine rotation, the grid lines should stay shown until I release the mouse button. My original approach using the timeout isn't quite the right one for that use case, and as you've flagged, the timeout itself can make things sluggish. I'd probably change the fade / timeout to 200ms, but otherwise what you've got in that screengrab looks promising! Did you want to go ahead and commit it? Code is cheap, and I think you're onto a better approach with the |
Contributor
Author
Oh, and just to answer this question, yes that's the intended UX. My thinking for this feature was: While I'm interacting with controls, I want to see grid lines so that I can be sure I'm aligning things nicely to a grid (e.g. fine grained rotation to line up a horizon). But when I'm not interacting with the controls, I want to see the image as it looks in its final(ish) state. So your idea I think should fulfill that nicely! |
Member
Apologies, I switched branches and dumped it :( I can dig up the agent discussion if it helps. |
Contributor
Author
|
Not a worry at all! I'll have a play 😄 |
…can tell the cropper when an interaction is happening
andrewserong
force-pushed
the
update/image-cropper-interactive-grid-lines-state
branch
from
22e3510 to
8f2f176
Compare
Contributor
Author
|
Okay, I've had a play with a few different options and pushed one of them in 8f2f176, though I'm not too sold on the idea. Here's how it's looking: 2026-04-28.16.15.10.mp4It adds an additional API to
But I really don't love the additional API there, as it kind of just adds extra noise to the API contract, and it's really only there so that we don't have to have a long timeout and so that we can distinguish user interactions from programatic ones like "reset". In any case, just thought I'd push what I've got right now in case you have any thoughts or opinions and since I got a little stuck down this particular rabbit hole! I'd love to try to make this simpler if we can. One way to simplify would be to roll this back considerably and still use the implicit state change approach (i.e. no exposed API) but:
What do you think? |
Member
|
Thanks for the updates @andrewserong 🍺 I've been playing with it, and manually it getting closer. I think I managed to find a way to keep the grid on permanently 🤣 2026-04-28.16.31.48.mp4I'll take a more detailed look tomorrow. I'll also see if I can regen my demo code to compare. I don't think it was much different to your approach TBH, but maybe there's a compromise/3rd way |
Contributor
Author
|
Sounds good, cheers! Worst case we roll back to the simplest internal state change / useEffect we can and try to skip when changing from isDirty to not dirty. |
Contributor
Author
😆 I think that might be a drawback of trying to always keep it on until the end action fires. A debounce/timer based approach is a bit safer because you then don't need to worry about what happens after someone interacts 🤔 |
Contributor
Author
I've opened a simpler alternative in #77735 — I think something like that might be a pragmatic balance for now. As always, happy to throw out any of these ideas, code is cheap! |
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
2 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.
Part of:
What?
Allow an interactive state for the grid lines of the image editor, so that the grid lines only show after interaction, but are otherwise hidden
Why?
Gridlines are really useful for things like fine grained rotation, but otherwise can interfere with looking at the image. This PR explores "interactive" as a default mode for the grid lines to see how it feels
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
2026-04-27.16.58.23.mp4
Use of AI Tools
Claude Code