Feat/width height guides #3021
Feat/width height guides #3021
Conversation
|
@RestartDK is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR adds a 5px deadzone before activating resize, computes per-mouse deltas, integrates dimension-based snapping into the resize flow (width/height), displays/hides snap lines appropriately, and ensures snap lines are hidden on resize stop. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ResizeHandler as resize-handles.tsx
participant SnapModule as snap/index.ts
participant Canvas
User->>ResizeHandler: pointerdown -> start resize
ResizeHandler->>ResizeHandler: track movement (dx,dy)
alt movement > 5px deadzone
ResizeHandler->>ResizeHandler: isResizeActive = true
alt snapping enabled && no Ctrl/Meta
ResizeHandler->>SnapModule: calculateDimensionSnapTarget(frameId, dim, pos, resizingDimensions)
SnapModule-->>ResizeHandler: {dimension?, snapLines?} or null
alt returned snap target
ResizeHandler->>Canvas: apply snapped dimension (clamped)
ResizeHandler->>Canvas: show snapLines / refresh overlays
else no snap
ResizeHandler->>Canvas: apply direct dimension update
ResizeHandler->>Canvas: hide snapLines
end
else
ResizeHandler->>Canvas: apply direct dimension update
ResizeHandler->>Canvas: hide snapLines
end
else within deadzone
ResizeHandler->>ResizeHandler: wait (do not resize)
end
User->>ResizeHandler: pointerup -> stopResize
ResizeHandler->>Canvas: hide snapLines, finalize
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
|
||
| // Check deadzone - only start resizing after 5px movement | ||
| if (!isResizeActive) { | ||
| if (dx * dx + dy * dy <= 25) { |
There was a problem hiding this comment.
Consider extracting the deadzone threshold (currently hardcoded as 25) into a named constant or configuration for easier future tuning.
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
Outdated
Show resolved
Hide resolved
![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
🧹 Nitpick comments (3)
apps/web/client/src/components/store/editor/snap/index.ts (1)
255-278: Anchor dimension guides to the matched frame’s edge for consistency with alignment linesCurrently the guide position uses the drag frame’s edges; align to the matched frame’s edges for consistent visuals and to better reflect “where the match occurs.”
- const widthLine = this.createSnapLine( + const widthLine = this.createSnapLine( SnapLineType.EDGE_RIGHT, 'vertical', - dragBounds.right, + otherFrame.bounds.right, otherFrame, dragBounds, ); ... - const heightLine = this.createSnapLine( + const heightLine = this.createSnapLine( SnapLineType.EDGE_BOTTOM, 'horizontal', - dragBounds.bottom, + otherFrame.bounds.bottom, otherFrame, dragBounds, );apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx (2)
6-6: Use native MouseEvent types for window listeners; drop unsafe castsThe handlers are registered on window, but typed as React.MouseEvent and then force‑cast to EventListener. Use ReactMouseEvent only for the initial onMouseDown; use global MouseEvent for window events.
-import type { MouseEvent } from 'react'; +import type { MouseEvent as ReactMouseEvent } from 'react'; ... - const startResize = (e: MouseEvent, types: HandleType[]) => { + const startResize = (e: ReactMouseEvent, types: HandleType[]) => { ... - const resize = (e: MouseEvent) => { + const resize = (e: globalThis.MouseEvent) => { ... - const stopResize = (e: MouseEvent) => { + const stopResize = (e: globalThis.MouseEvent) => { ... - window.removeEventListener('mousemove', resize as unknown as EventListener); - window.removeEventListener('mouseup', stopResize as unknown as EventListener); + window.removeEventListener('mousemove', resize); + window.removeEventListener('mouseup', stopResize); ... - window.addEventListener('mousemove', resize as unknown as EventListener); - window.addEventListener('mouseup', stopResize as unknown as EventListener); + window.addEventListener('mousemove', resize); + window.addEventListener('mouseup', stopResize);Also applies to: 20-21, 32-32, 115-122, 124-125
100-109: Consolidate hideSnapLines callshideSnapLines() is called in three branches; collapse to one call to reduce churn.
- } else { - editorEngine.snap.hideSnapLines(); - } - } else { - editorEngine.snap.hideSnapLines(); - } - - // No snapping or snapping disabled - editorEngine.snap.hideSnapLines(); + } else { + // fall through to non-snap path below + } + } + // No snapping or snapping disabled + editorEngine.snap.hideSnapLines();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx(2 hunks)apps/web/client/src/components/store/editor/snap/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/snap/index.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/snap/index.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/snap/index.tsapps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
🧬 Code graph analysis (1)
apps/web/client/src/components/store/editor/snap/index.ts (1)
apps/web/client/src/components/store/editor/snap/types.ts (1)
SnapLine(20-28)
🔇 Additional comments (4)
apps/web/client/src/components/store/editor/snap/index.ts (1)
284-375: Dimension snapping selection logic looks solidClosest-by-dimension within threshold, optional per-axis gating, and line generation are coherent. No blocking issues from this block.
If multiple frames share identical closest differences, current tie-breaking is by iteration order. Confirm that’s acceptable or sort by a stable key to avoid UI flicker.
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx (3)
33-43: Deadzone uses screen pixels; confirm UX intent across zoom levelsThe 5px threshold is in device pixels (pre‑scale). If the desired feel is canvas‑space consistency, consider dividing by scale.
13-15: Observer component needs a client boundaryPer guidelines, observer components must be client components. Ensure a parent file in this feature sets 'use client', or add it here if this is the entry boundary.
119-119: Good: cleanup hides guides on stopSnap lines are cleared on mouseup; this prevents stale guides.
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx (3)
33-43: Extract the deadzone threshold to a named constant/config.Hardcoded 5px makes tuning harder; hoist to a constant and reuse.
+const DEADZONE_PX = 5; @@ - // Check deadzone - only start resizing after 5px movement + // Check deadzone - only start resizing after DEADZONE_PX movement if (!isResizeActive) { - if (dx * dx + dy * dy <= 25) { + if (dx * dx + dy * dy <= DEADZONE_PX * DEADZONE_PX) { return; // Still within deadzone } isResizeActive = true; }
80-116: Remove redundant hideSnapLines() call when no snap target.You hide lines inside the else and again in the fallback path.
if (dimensionSnapTarget) { // Apply snapped dimensions @@ editorEngine.overlay.undebouncedRefresh(); return; - } else { - editorEngine.snap.hideSnapLines(); } } @@ - // No snapping or snapping disabled + // No snapping or snapping disabled editorEngine.snap.hideSnapLines(); editorEngine.frames.updateAndSaveToStorage(frame.id, { dimension: { width: Math.round(newWidth), height: Math.round(newHeight) }, });
92-108: Nice: snapped path clamps to min size and manages guides accordingly.This resolves the prior min-size bypass.
🧹 Nitpick comments (3)
apps/web/client/src/components/store/editor/snap/index.ts (2)
25-36: Broaden alignment gating to axis overlap (not just top/left equality).Current gating treats frames as “aligned” only when top/left edges are within threshold, which may miss legitimate width/height matches when frames are offset but overlap along an axis. Prefer overlap-based gating per axis.
Suggested change: pass current dimension and compute axis overlap.
- private isAlignedWithFrame(currentPosition: RectPosition, otherFrame: SnapFrame): boolean { - // Check if frames are horizontally aligned for width snapping - const yDifference = Math.abs(currentPosition.y - otherFrame.bounds.top); - const isHorizontallyAligned = yDifference <= this.config.threshold; - - // Check if frames are vertically aligned for height snapping - const xDifference = Math.abs(currentPosition.x - otherFrame.bounds.left); - const isVerticallyAligned = xDifference <= this.config.threshold; - - // Frame is aligned if it's either horizontally OR vertically aligned - return isHorizontallyAligned || isVerticallyAligned; - } + private isAlignedWithFrame( + currentPosition: RectPosition, + currentDimension: RectDimension, + otherFrame: SnapFrame, + ): boolean { + const currTop = currentPosition.y; + const currBottom = currentPosition.y + currentDimension.height; + const currLeft = currentPosition.x; + const currRight = currentPosition.x + currentDimension.width; + + // Axis overlap with tolerance + const verticalOverlap = + currBottom >= otherFrame.bounds.top - this.config.threshold && + currTop <= otherFrame.bounds.bottom + this.config.threshold; + const horizontalOverlap = + currRight >= otherFrame.bounds.left - this.config.threshold && + currLeft <= otherFrame.bounds.right + this.config.threshold; + + // Overlap on either axis is sufficient (width or height guides) + return verticalOverlap || horizontalOverlap; + } @@ - const alignedFrames = allFrames.filter(frame => this.isAlignedWithFrame(position, frame)); + const alignedFrames = allFrames.filter(frame => this.isAlignedWithFrame(position, dimension, frame));Also applies to: 266-271
228-236: Use stable snap line IDs to reduce overlay churn/flicker.Date.now() changes every move; stable IDs improve keyed renders.
- return { - id: `${type}-${otherFrame.id}-${Date.now()}`, + return { + id: `${type}-${otherFrame.id}-${orientation}-${Math.round(position)}`, type, orientation, position, start, end, frameIds: [otherFrame.id], };apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx (1)
20-21: Type handlers for DOM events; avoid casting React MouseEvent to EventListener.Use ReactMouseEvent only for onMouseDown; use global MouseEvent for window listeners to remove unsafe casts.
-import type { MouseEvent } from 'react'; +import type { MouseEvent as ReactMouseEvent } from 'react'; @@ - const startResize = (e: MouseEvent, types: HandleType[]) => { + const startResize = (e: ReactMouseEvent, types: HandleType[]) => { @@ - const resize = (e: MouseEvent) => { + const resize = (e: globalThis.MouseEvent) => { @@ - const stopResize = (e: MouseEvent) => { + const stopResize = (e: globalThis.MouseEvent) => { @@ - window.removeEventListener('mousemove', resize as unknown as EventListener); - window.removeEventListener('mouseup', stopResize as unknown as EventListener); + window.removeEventListener('mousemove', resize); + window.removeEventListener('mouseup', stopResize); @@ - window.addEventListener('mousemove', resize as unknown as EventListener); - window.addEventListener('mouseup', stopResize as unknown as EventListener); + window.addEventListener('mousemove', resize); + window.addEventListener('mouseup', stopResize);Also applies to: 32-43, 122-129, 131-133
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx(2 hunks)apps/web/client/src/components/store/editor/snap/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsxapps/web/client/src/components/store/editor/snap/index.ts
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsxapps/web/client/src/components/store/editor/snap/index.ts
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsxapps/web/client/src/components/store/editor/snap/index.ts
🧬 Code graph analysis (1)
apps/web/client/src/components/store/editor/snap/index.ts (1)
apps/web/client/src/components/store/editor/snap/types.ts (2)
SnapFrame(40-45)SnapLine(20-28)
🔇 Additional comments (1)
apps/web/client/src/app/project/[id]/_components/canvas/frame/resize-handles.tsx (1)
1-1: Confirm client boundary for this observer component.This component uses events and window APIs; ensure an ancestor declares 'use client'. If not, add it here.
+'use client';
1 participant
Description
This PR allows the user to:
Related Issues
closes #2909
Type of Change
Testing
No tests were made for this however I resized the frame using 2 - 4 frames at the same time, seeing if anything would conflict.
Screenshots (if applicable)
onlook-1.mp4
Additional Notes
For this feature I reused the current designs for the lines that are the same as the alignment ones. Please let me know if this needs to be changed to be more in line with the initial screenshot from #2909.
Important
Adds width and height resizing guides with snapping functionality to frames, enhancing user experience with visual feedback and alignment in
resize-handles.tsxandsnap/index.ts.resize-handles.tsx.calculateDimensionSnapTarget()anddetectDimensionAlignment()to handle snapping logic insnap/index.ts.createSnapBounds()andgetSnapFrames()to determine snapping candidates.isResizeActiveto handle deadzone logic inresize-handles.tsx.resize-handles.tsx.This description was created by
for ac587cb. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit