Draft
Conversation
…city control - Added `showGrid` prop to `Cropper` for interactive grid display during placement interactions. - Implemented grid visibility management with fade effects based on user interactions. - Updated `GridOverlay` to accept an `opacity` prop for customizable grid transparency. - Enhanced `RectangleStencil` to manage keyboard resize interactions more effectively. This update improves the user experience by providing visual feedback during cropping actions.
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an “interactive” grid mode to the Media Editor Cropper so the rule-of-thirds grid appears during placement-oriented interactions (pan/zoom/keyboard nudging/resize) and fades out otherwise.
Changes:
- Extends
Cropper’sshowGridprop to supportfalse | true | 'interactive'and implements fade-in/out behavior for interactive mode. - Introduces new interaction signals (
isPlacementInteracting, keyboard-resize start/end) to keep the grid visible for the full duration of drag/resize and for brief keyboard placement bursts. - Enables interactive grid mode in the modal canvas and adds a Storybook control to toggle grid modes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/media-editor/src/image-editor/stories/rectangle-crop.story.tsx | Adds a Storybook select to toggle grid modes and wires it to Cropper.showGrid. |
| packages/media-editor/src/image-editor/react/hooks/use-interaction.ts | Adds isPlacementInteracting with keyboard-idle tracking for placement interactions. |
| packages/media-editor/src/image-editor/react/components/stencils/rectangle-stencil.tsx | Signals resize start/end for keyboard resizing to support interaction-driven UI (grid visibility). |
| packages/media-editor/src/image-editor/react/components/overlays/grid-overlay.tsx | Adds optional opacity prop for grid fade behavior. |
| packages/media-editor/src/image-editor/react/components/cropper.tsx | Implements 'interactive' grid visibility logic and hooks resize start into gesture/grid signaling. |
| packages/media-editor/src/image-editor/react/components/cropper.scss | Adds opacity transition to support grid fading. |
| packages/media-editor/src/components/media-editor-canvas/index.tsx | Sets showGrid="interactive" in the modal canvas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
113
to
+136
|
|
||
| const startPlacementInteracting = useCallback( () => { | ||
| clearTimeout( keyboardInteractionTimerRef.current ); | ||
| setIsPlacementInteracting( true ); | ||
| }, [] ); | ||
|
|
||
| const stopPlacementInteracting = useCallback( () => { | ||
| clearTimeout( keyboardInteractionTimerRef.current ); | ||
| setIsPlacementInteracting( false ); | ||
| }, [] ); | ||
|
|
||
| const signalKeyboardPlacement = useCallback( () => { | ||
| setIsPlacementInteracting( true ); | ||
| clearTimeout( keyboardInteractionTimerRef.current ); | ||
| keyboardInteractionTimerRef.current = setTimeout( () => { | ||
| setIsPlacementInteracting( false ); | ||
| }, KEYBOARD_INTERACTION_IDLE_MS ); | ||
| }, [] ); | ||
|
|
||
| useEffect( () => { | ||
| return () => { | ||
| clearTimeout( keyboardInteractionTimerRef.current ); | ||
| }; | ||
| }, [] ); |
There was a problem hiding this comment.
useInteraction now adds keyboard-based placement tracking (isPlacementInteracting) with an idle timeout, but there are no hook tests covering the new timing/cleanup behavior (e.g., Arrow-key sets true, resets after the idle delay, timers cleared on unmount, drag status toggles). Since packages/media-editor/src/image-editor/react/hooks/test/ already covers other hooks, please add a focused test suite for use-interaction to prevent regressions in this interaction state machine.
Suggested change
| const startPlacementInteracting = useCallback( () => { | |
| clearTimeout( keyboardInteractionTimerRef.current ); | |
| setIsPlacementInteracting( true ); | |
| }, [] ); | |
| const stopPlacementInteracting = useCallback( () => { | |
| clearTimeout( keyboardInteractionTimerRef.current ); | |
| setIsPlacementInteracting( false ); | |
| }, [] ); | |
| const signalKeyboardPlacement = useCallback( () => { | |
| setIsPlacementInteracting( true ); | |
| clearTimeout( keyboardInteractionTimerRef.current ); | |
| keyboardInteractionTimerRef.current = setTimeout( () => { | |
| setIsPlacementInteracting( false ); | |
| }, KEYBOARD_INTERACTION_IDLE_MS ); | |
| }, [] ); | |
| useEffect( () => { | |
| return () => { | |
| clearTimeout( keyboardInteractionTimerRef.current ); | |
| }; | |
| }, [] ); | |
| const isMountedRef = useRef( true ); | |
| const clearKeyboardInteractionTimer = useCallback( () => { | |
| if ( keyboardInteractionTimerRef.current ) { | |
| clearTimeout( keyboardInteractionTimerRef.current ); | |
| keyboardInteractionTimerRef.current = null; | |
| } | |
| }, [] ); | |
| const startPlacementInteracting = useCallback( () => { | |
| clearKeyboardInteractionTimer(); | |
| setIsPlacementInteracting( true ); | |
| }, [ clearKeyboardInteractionTimer ] ); | |
| const stopPlacementInteracting = useCallback( () => { | |
| clearKeyboardInteractionTimer(); | |
| setIsPlacementInteracting( false ); | |
| }, [ clearKeyboardInteractionTimer ] ); | |
| const signalKeyboardPlacement = useCallback( () => { | |
| setIsPlacementInteracting( true ); | |
| clearKeyboardInteractionTimer(); | |
| keyboardInteractionTimerRef.current = setTimeout( () => { | |
| keyboardInteractionTimerRef.current = null; | |
| if ( isMountedRef.current ) { | |
| setIsPlacementInteracting( false ); | |
| } | |
| }, KEYBOARD_INTERACTION_IDLE_MS ); | |
| }, [ clearKeyboardInteractionTimer ] ); | |
| useEffect( () => { | |
| return () => { | |
| isMountedRef.current = false; | |
| clearKeyboardInteractionTimer(); | |
| }; | |
| }, [ clearKeyboardInteractionTimer ] ); |
Comment on lines
+381
to
+398
|
|
||
| const showInteractiveGrid = useCallback( () => { | ||
| if ( showGrid !== 'interactive' || ! gridInteractionReadyRef.current ) { | ||
| return; | ||
| } | ||
| clearTimeout( gridFadeTimerRef.current ); | ||
| setGridVisible( true ); | ||
| }, [ showGrid ] ); | ||
|
|
||
| const fadeInteractiveGridSoon = useCallback( () => { | ||
| if ( showGrid !== 'interactive' || ! gridInteractionReadyRef.current ) { | ||
| return; | ||
| } | ||
| clearTimeout( gridFadeTimerRef.current ); | ||
| gridFadeTimerRef.current = setTimeout( () => { | ||
| setGridVisible( false ); | ||
| }, GRID_FADE_DELAY_MS ); | ||
| }, [ showGrid ] ); |
There was a problem hiding this comment.
showInteractiveGrid is gated on gridInteractionReadyRef.current, which is only flipped to true via setTimeout(..., 0) after state.image is set. This can suppress the grid for a real user-initiated gesture that starts immediately after the image loads (before the timeout fires). Consider making user-driven interactions (drag/resize/keyboard) bypass the readiness gate (or set readiness synchronously once an image exists) while still preventing programmatic initialization updates from flashing the grid.
Suggested change
| const showInteractiveGrid = useCallback( () => { | |
| if ( showGrid !== 'interactive' || ! gridInteractionReadyRef.current ) { | |
| return; | |
| } | |
| clearTimeout( gridFadeTimerRef.current ); | |
| setGridVisible( true ); | |
| }, [ showGrid ] ); | |
| const fadeInteractiveGridSoon = useCallback( () => { | |
| if ( showGrid !== 'interactive' || ! gridInteractionReadyRef.current ) { | |
| return; | |
| } | |
| clearTimeout( gridFadeTimerRef.current ); | |
| gridFadeTimerRef.current = setTimeout( () => { | |
| setGridVisible( false ); | |
| }, GRID_FADE_DELAY_MS ); | |
| }, [ showGrid ] ); | |
| const canUseInteractiveGrid = | |
| gridInteractionReadyRef.current || | |
| ( !! state.image && ( isCropperInteracting || gridVisible ) ); | |
| const showInteractiveGrid = useCallback( () => { | |
| if ( showGrid !== 'interactive' || ! canUseInteractiveGrid ) { | |
| return; | |
| } | |
| clearTimeout( gridFadeTimerRef.current ); | |
| setGridVisible( true ); | |
| }, [ canUseInteractiveGrid, showGrid ] ); | |
| const fadeInteractiveGridSoon = useCallback( () => { | |
| if ( showGrid !== 'interactive' || ! canUseInteractiveGrid ) { | |
| return; | |
| } | |
| clearTimeout( gridFadeTimerRef.current ); | |
| gridFadeTimerRef.current = setTimeout( () => { | |
| setGridVisible( false ); | |
| }, GRID_FADE_DELAY_MS ); | |
| }, [ canUseInteractiveGrid, showGrid ] ); |
Comment on lines
+390
to
+399
| const fadeInteractiveGridSoon = useCallback( () => { | ||
| if ( showGrid !== 'interactive' || ! gridInteractionReadyRef.current ) { | ||
| return; | ||
| } | ||
| clearTimeout( gridFadeTimerRef.current ); | ||
| gridFadeTimerRef.current = setTimeout( () => { | ||
| setGridVisible( false ); | ||
| }, GRID_FADE_DELAY_MS ); | ||
| }, [ showGrid ] ); | ||
|
|
There was a problem hiding this comment.
When switching showGrid away from 'interactive' (e.g., story control toggles), any pending gridFadeTimerRef timeout can still fire and call setGridVisible(false) even though the grid is no longer in interactive mode. Consider clearing the fade timer (and optionally resetting gridVisible) in a useEffect that runs when showGrid changes, to avoid stray state updates/rerenders after a mode switch.
|
Size Change: +648 B (+0.01%) Total Size: 7.82 MB 📦 View Changed
ℹ️ View Unchanged
|
…d management - Replaced inline grid visibility logic in the Cropper with the new `useInteractiveGrid` hook. - Simplified state management for grid visibility during placement interactions. - Enhanced responsiveness of the grid display based on user interactions, including resize events.
…iming - Adjusted the transition timing for grid opacity from 0.2s to 0.15s for a snappier user experience. - Updated the `useInteractiveGrid` hook to simplify grid visibility management by removing resize notifications and integrating resizing state directly within the Cropper component. - Enhanced interaction handling by introducing a local resizing state to better manage cropper interactions.
ramonjd
commented
Apr 28, 2026
| @@ -125,14 +170,19 @@ export function useInteraction( | |||
| onStatusChange: ( status ) => { | |||
| setIsDragging( status.isDragging ); | |||
Member
Author
There was a problem hiding this comment.
Double check pinch and zoom.
Pinch zoom can lose the grid while the touch gesture is still active
use-interaction.ts (line 170) only drives isPlacementInteracting from status.isDragging. Two-finger pinch zoom uses gesture start/end internally, but it never sets isDragging, so use-interactive-grid.ts (line 110) falls back to value-change flashing. If the user pauses with two fingers still down, the grid can fade after 100ms even though zoom placement is still active. Assuming zoom is intended as a placement operation, pinch should probably hold the grid open for the full gesture.
ramonjd
commented
Apr 28, 2026
| previous.cropY !== current.cropY || | ||
| previous.cropWidth !== current.cropWidth || | ||
| previous.cropHeight !== current.cropHeight; | ||
| const orientationChanged = |
Member
Author
There was a problem hiding this comment.
Fine rotation is currently excluded because any state.rotation change is treated as orientationChanged in use-interactive-grid.ts (line 133). That matches “don’t show for rotate” if fine rotate is grouped with snap rotate, but if fine rotate is considered a composition/placement adjustment, this implementation will need a separate signal.
|
Flaky tests detected in 6b399a7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25046382941
|
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
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.
The grid appears for placement-oriented interactions like crop, pan, zoom, and keyboard nudging, then fades out.
It avoids showing for non-placement actions such as flip, reset, rotate, and aspect ratio changes. Fine rotate was left out for now, but that's debatable?
Keeps the grid visible for the full duration of drag/resize gestures instead of tying visibility only to value changes.
Kapture.2026-04-28.at.18.58.44.mp4