Improve corner radius handle UX and positioning #501
Merged
+227
−31
Conversation
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
… for uniform adjustments
|
@ryujunghy3on is attempting to deploy a commit to the Grida Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds Alt key modifier support to corner radius gestures in the canvas editor. The altKey state is threaded from the UI drag handler through the action dispatch layer and reducers, enabling conditional logic where pressing Alt maintains uniform corner radius updates versus individual corner adjustments. The corner-radius-handle component is optimized with useMemo for performance. Changes
Sequence DiagramsequenceDiagram
participant UI as Corner Radius Handle
participant API as Editor API
participant Dispatch as Action Dispatch
participant Reducer as Reducers
participant State as Gesture State
UI->>UI: onDragStart detects Alt key
UI->>API: surfaceStartCornerRadiusGesture(selection, anchor, altKey)
API->>Dispatch: dispatch surface/gesture/start
Dispatch->>Reducer: corner-radius gesture payload with altKey
Reducer->>Reducer: destructure altKey from gesture
Reducer->>State: store altKey in draft.gesture.altKey
Reducer->>Reducer: on drag: check altKey & corner uniformity
alt altKey unpressed AND corners uniform
Reducer->>State: update unified corner_radius + all rectangular variants
else altKey pressed OR non-uniform
Reducer->>State: update individual corner (nw/ne/se/sw)
end
State-->>UI: reflect updated radius values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/grida-canvas/reducers/event-target.reducer.ts (1)
735-825: The Alt-key behavior contradicts the stated feature goal.The commit message says "altKey support for uniform adjustments," but the current implementation forces per-corner adjustments when Alt is held. Either the condition should be flipped to
altKey || isUniform(if Alt should enforce uniformity), or the feature needs clarification on the intended UX.🔧 Suggested fix (if Alt should enforce uniform updates)
- if (!altKey && isUniform) { + if (altKey || isUniform) {Please verify the intended behavior and update accordingly.
🤖 Fix all issues with AI agents
In `@editor/grida-canvas-react/viewport/ui/corner-radius-handle.tsx`:
- Around line 181-189: The labelPositions object uses labelOffsets.X for bottom
vertical offsets in the 'se' and 'sw' entries causing bottom labels to use the X
constant instead of the vertical Y offset; update the 'se' and 'sw' entries to
use labelOffsets.Y_BOTTOM for their bottom values (keeping left/right as
labelOffsets.X and top entries using labelOffsets.Y_TOP), so the returned value
from labelPositions[anchor] places bottom labels with the correct vertical
spacing (references: labelPositions, anchor, labelOffsets.Y_BOTTOM,
labelOffsets.Y_TOP, minmargin).
- Around line 60-138: The geometry calculation currently compares an unscaled
currentRadius against pixel margins, which breaks under zoom/anisotropic
transforms; update geometry's radius math to compute a scaled radius in pixels
(e.g., scaledRadiusX = currentRadius * scaleX, scaledRadiusY = currentRadius *
scaleY, scaledRadiusPx = Math.max(|scaledRadiusX|, |scaledRadiusY|)), then use
scaledRadiusPx when computing minmargin and useMarginBased (minmargin =
Math.max(scaledRadiusPx + size, margin); useMarginBased = scaledRadiusPx <
margin). Keep the existing offsets (they already use currentRadius *
scaleX/scaleY) and ensure these renamed/added scaled values are used in the same
useMemo that defines minmargin/useMarginBased and referenced by handleStyle.
Comment on lines
+60
to
+138
| const geometry = useMemo(() => { | ||
| const br = editor.geometryProvider.getNodeAbsoluteBoundingRect(node_id); | ||
| if (!br) return null; | ||
|
|
||
| const boundingSurfaceRect = cmath.rect.transform(br, transform); | ||
| const [scaleX, scaleY] = cmath.transform.getScale(transform); | ||
| const w = boundingSurfaceRect.width; | ||
| const h = boundingSurfaceRect.height; | ||
| const minmargin = Math.max(currentRadius + size, margin); | ||
| const useMarginBased = currentRadius < margin; | ||
|
|
||
| // Corner coordinates: C = (C_x, C_y) | ||
| const corners = { | ||
| nw: [0, 0], | ||
| ne: [w, 0], | ||
| se: [w, h], | ||
| sw: [0, h], | ||
| } as const; | ||
| const [C_x, C_y] = corners[anchor]; | ||
|
|
||
| // Arc center offset: O = (O_x, O_y) = (r * s_x * sign_x, r * s_y * sign_y) | ||
| const offsets = { | ||
| nw: [currentRadius * scaleX, currentRadius * scaleY], | ||
| ne: [-currentRadius * scaleX, currentRadius * scaleY], | ||
| se: [-currentRadius * scaleX, -currentRadius * scaleY], | ||
| sw: [currentRadius * scaleX, -currentRadius * scaleY], | ||
| } as const; | ||
| const [O_x, O_y] = offsets[anchor]; | ||
|
|
||
| // Center coordinates: M = (M_x, M_y) = (w/2, h/2) | ||
| const M_x = w / 2; | ||
| const M_y = h / 2; | ||
|
|
||
| // Handle position relative to center: H = (H_x, H_y) = (C + O - M) | ||
| const H_x = C_x + O_x - M_x; | ||
| const H_y = C_y + O_y - M_y; | ||
|
|
||
| return { | ||
| w, | ||
| h, | ||
| scaleX, | ||
| scaleY, | ||
| minmargin, | ||
| useMarginBased, | ||
| H_x, | ||
| H_y, | ||
| M_x, | ||
| M_y, | ||
| }; | ||
| }, [editor.geometryProvider, node_id, anchor, currentRadius, transform, size, margin]); | ||
|
|
||
| // Calculate handle position: at arc center O when radius >= margin, otherwise at corner with margin | ||
| const handleStyle = useMemo(() => { | ||
| if (!geometry) return null; | ||
|
|
||
| const { useMarginBased, H_x, H_y, minmargin } = geometry; | ||
|
|
||
| if (!useMarginBased && currentRadius > 0) { | ||
| // Handle at arc center: H = (C + O - M) relative to center | ||
| return { | ||
| left: `calc(50% + ${H_x}px)`, | ||
| top: `calc(50% + ${H_y}px)`, | ||
| transform: "translate(-50%, -50%)", | ||
| }; | ||
| } | ||
|
|
||
| // Handle at corner with margin: position = minmargin from edge | ||
| const positions = { | ||
| nw: { top: `${minmargin}px`, left: `${minmargin}px` }, | ||
| ne: { top: `${minmargin}px`, right: `${minmargin}px` }, | ||
| se: { bottom: `${minmargin}px`, right: `${minmargin}px` }, | ||
| sw: { bottom: `${minmargin}px`, left: `${minmargin}px` }, | ||
| } as const; | ||
|
|
||
| return { | ||
| ...positions[anchor], | ||
| transform: `translate(${anchor[1] === "w" ? "-50%" : "50%"}, ${anchor[0] === "n" ? "-50%" : "50%"})`, | ||
| }; | ||
| }, [geometry, anchor, currentRadius]); |
There was a problem hiding this comment.
Scale-aware margin logic needed for zoomed/anisotropic transforms.
useMarginBased / minmargin compare an unscaled radius against pixel margins, so at zoom ≠ 1 the handle can jump to the wrong mode.
🧩 Suggested fix (use scaled radius in px)
- const minmargin = Math.max(currentRadius + size, margin);
- const useMarginBased = currentRadius < margin;
+ const radiusPx = Math.min(
+ Math.abs(currentRadius * scaleX),
+ Math.abs(currentRadius * scaleY)
+ );
+ const minmargin = Math.max(radiusPx + size, margin);
+ const useMarginBased = radiusPx < margin;
@@
- return {
+ return {
w,
h,
scaleX,
scaleY,
+ radiusPx,
minmargin,
useMarginBased,
H_x,
H_y,
M_x,
M_y,
};
@@
- const { useMarginBased, H_x, H_y, minmargin } = geometry;
+ const { useMarginBased, H_x, H_y, minmargin, radiusPx } = geometry;
@@
- if (!useMarginBased && currentRadius > 0) {
+ if (!useMarginBased && radiusPx > 0) {🤖 Prompt for AI Agents
In `@editor/grida-canvas-react/viewport/ui/corner-radius-handle.tsx` around lines
60 - 138, The geometry calculation currently compares an unscaled currentRadius
against pixel margins, which breaks under zoom/anisotropic transforms; update
geometry's radius math to compute a scaled radius in pixels (e.g., scaledRadiusX
= currentRadius * scaleX, scaledRadiusY = currentRadius * scaleY, scaledRadiusPx
= Math.max(|scaledRadiusX|, |scaledRadiusY|)), then use scaledRadiusPx when
computing minmargin and useMarginBased (minmargin = Math.max(scaledRadiusPx +
size, margin); useMarginBased = scaledRadiusPx < margin). Keep the existing
offsets (they already use currentRadius * scaleX/scaleY) and ensure these
renamed/added scaled values are used in the same useMemo that defines
minmargin/useMarginBased and referenced by handleStyle.
Comment on lines
+181
to
+189
| // Margin-based: label inside direction from handle | ||
| const labelPositions = { | ||
| nw: { top: `${minmargin + labelOffsets.Y_TOP}px`, left: `${minmargin + labelOffsets.X}px` }, | ||
| ne: { top: `${minmargin + labelOffsets.Y_TOP}px`, right: `${minmargin + labelOffsets.X}px` }, | ||
| se: { bottom: `${minmargin + labelOffsets.X}px`, right: `${minmargin + labelOffsets.X}px` }, | ||
| sw: { bottom: `${minmargin + labelOffsets.X}px`, left: `${minmargin + labelOffsets.X}px` }, | ||
| } as const; | ||
|
|
||
| return labelPositions[anchor]; |
There was a problem hiding this comment.
Bottom label offsets use the X constant (likely typo).
For se/sw, the vertical offset should probably use Y_BOTTOM to match the top-corner spacing and avoid the label sitting too close.
🔧 Suggested fix
- se: { bottom: `${minmargin + labelOffsets.X}px`, right: `${minmargin + labelOffsets.X}px` },
- sw: { bottom: `${minmargin + labelOffsets.X}px`, left: `${minmargin + labelOffsets.X}px` },
+ se: { bottom: `${minmargin + labelOffsets.Y_BOTTOM}px`, right: `${minmargin + labelOffsets.X}px` },
+ sw: { bottom: `${minmargin + labelOffsets.Y_BOTTOM}px`, left: `${minmargin + labelOffsets.X}px` },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Margin-based: label inside direction from handle | |
| const labelPositions = { | |
| nw: { top: `${minmargin + labelOffsets.Y_TOP}px`, left: `${minmargin + labelOffsets.X}px` }, | |
| ne: { top: `${minmargin + labelOffsets.Y_TOP}px`, right: `${minmargin + labelOffsets.X}px` }, | |
| se: { bottom: `${minmargin + labelOffsets.X}px`, right: `${minmargin + labelOffsets.X}px` }, | |
| sw: { bottom: `${minmargin + labelOffsets.X}px`, left: `${minmargin + labelOffsets.X}px` }, | |
| } as const; | |
| return labelPositions[anchor]; | |
| // Margin-based: label inside direction from handle | |
| const labelPositions = { | |
| nw: { top: `${minmargin + labelOffsets.Y_TOP}px`, left: `${minmargin + labelOffsets.X}px` }, | |
| ne: { top: `${minmargin + labelOffsets.Y_TOP}px`, right: `${minmargin + labelOffsets.X}px` }, | |
| se: { bottom: `${minmargin + labelOffsets.Y_BOTTOM}px`, right: `${minmargin + labelOffsets.X}px` }, | |
| sw: { bottom: `${minmargin + labelOffsets.Y_BOTTOM}px`, left: `${minmargin + labelOffsets.X}px` }, | |
| } as const; | |
| return labelPositions[anchor]; |
🤖 Prompt for AI Agents
In `@editor/grida-canvas-react/viewport/ui/corner-radius-handle.tsx` around lines
181 - 189, The labelPositions object uses labelOffsets.X for bottom vertical
offsets in the 'se' and 'sw' entries causing bottom labels to use the X constant
instead of the vertical Y offset; update the 'se' and 'sw' entries to use
labelOffsets.Y_BOTTOM for their bottom values (keeping left/right as
labelOffsets.X and top entries using labelOffsets.Y_TOP), so the returned value
from labelPositions[anchor] places bottom labels with the correct vertical
spacing (references: labelPositions, anchor, labelOffsets.Y_BOTTOM,
labelOffsets.Y_TOP, minmargin).
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.
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.