Added pinch-zoom.ts to make pinch zoom multiplier. It works. Will com… - #134
Added pinch-zoom.ts to make pinch zoom multiplier. It works. Will com…#134Bibli2311 wants to merge 2 commits into
Conversation
…e back to finalize
1432b00 to
71be538
Compare
There was a problem hiding this comment.
Pull request overview
Adds a custom pinch/ctrl+wheel zoom handler to React Flow to support “zoom-to-cursor” behavior and a “boosted” zoom speed when CTRL is pressed.
Changes:
- Added
lib/pinch-zoom.tshelper to compute zoom-to-cursor and update viewport viasetViewport/setCenter. - Wired a native
wheellistener incomponents/dig/flow.tsxto intercept ctrl+wheel and prevent browser page zoom. - Refactored some component code formatting and effect dependencies.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| lib/pinch-zoom.ts | Introduces a wheel-event handler that computes clamped zoom-to-cursor and applies it to React Flow viewport. |
| components/dig/flow.tsx | Attaches the native wheel listener to the flow container and routes events into the pinch zoom helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| getContainer, | ||
| minZoom, | ||
| maxZoom, | ||
| boost = 30000000000.0, |
There was a problem hiding this comment.
The default boost value (30000000000.0) combined with vp.zoom + delta makes zoom changes effectively explode/clamp on almost any wheel input, so zoom-to-cursor will “snap” to min/max rather than behave smoothly. Use a reasonable default (e.g., 1) and compute zoom multiplicatively (e.g., apply a factor derived from deltaY) rather than additively to vp.zoom.
| // compute delta (tweak multiplier if needed) | ||
| const delta = -e.deltaY * 0.002 * boost; |
There was a problem hiding this comment.
The default boost value (30000000000.0) combined with vp.zoom + delta makes zoom changes effectively explode/clamp on almost any wheel input, so zoom-to-cursor will “snap” to min/max rather than behave smoothly. Use a reasonable default (e.g., 1) and compute zoom multiplicatively (e.g., apply a factor derived from deltaY) rather than additively to vp.zoom.
| // compute new zoom, clamped | ||
| const newZoom = Math.max(minZoom, Math.min(maxZoom, vp.zoom + delta)); |
There was a problem hiding this comment.
The default boost value (30000000000.0) combined with vp.zoom + delta makes zoom changes effectively explode/clamp on almost any wheel input, so zoom-to-cursor will “snap” to min/max rather than behave smoothly. Use a reasonable default (e.g., 1) and compute zoom multiplicatively (e.g., apply a factor derived from deltaY) rather than additively to vp.zoom.
| // fallback: center zoom (keeps prior behavior) | ||
| if (typeof setViewport === 'function') { | ||
| setViewport({ ...vp, zoom: newZoom }); | ||
| } else if (typeof setCenter === 'function') { | ||
| // approximate: keep center same | ||
| setCenter(0, 0, { zoom: newZoom, duration: 0 }); |
There was a problem hiding this comment.
If container is null and setViewport is unavailable, the fallback setCenter(0, 0, ...) will jump the view to the graph origin, which is a clear behavior regression. Prefer a no-op in this case (return without changing anything), or compute the current center from the existing viewport + container size (which requires a container), rather than hardcoding (0, 0).
| // fallback: center zoom (keeps prior behavior) | |
| if (typeof setViewport === 'function') { | |
| setViewport({ ...vp, zoom: newZoom }); | |
| } else if (typeof setCenter === 'function') { | |
| // approximate: keep center same | |
| setCenter(0, 0, { zoom: newZoom, duration: 0 }); | |
| // fallback when container is unavailable: | |
| // if setViewport exists, apply zoom centered on current viewport; | |
| // otherwise, do nothing to avoid jumping to the origin. | |
| if (typeof setViewport === 'function') { | |
| setViewport({ ...vp, zoom: newZoom }); |
| // prevent browser page zoom | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
|
|
||
| // forward the DOM event to the handler (no React event types) | ||
| try { | ||
| pinchZoomHandler(e); | ||
| } catch (err) { | ||
| // swallow errors |
There was a problem hiding this comment.
Errors are silently swallowed, which makes failures difficult to diagnose (especially since this PR notes macOS issues). Consider at least logging in non-production (or rethrowing) so unexpected wheel/viewport errors don’t fail silently. Also, pinchZoomHandler already calls preventDefault()/stopPropagation(), so doing it here as well is duplicated—pick one place to own that behavior.
| // prevent browser page zoom | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| // forward the DOM event to the handler (no React event types) | |
| try { | |
| pinchZoomHandler(e); | |
| } catch (err) { | |
| // swallow errors | |
| // forward the DOM event to the handler (no React event types) | |
| try { | |
| pinchZoomHandler(e); | |
| } catch (err) { | |
| // log errors instead of swallowing them silently | |
| console.error('Error in pinch zoom handler:', err); |
| if (focusNodeId) { | ||
| focusNodeHandle(focusNodeId); | ||
| } | ||
| }, [focusNodeId]); // eslint-disable-line |
There was a problem hiding this comment.
The eslint disables are masking dependency issues rather than fixing them. For the focusNodeId effect, either include the real dependencies (or wrap focusNodeHandle in useCallback) so the closure is correct; for the useMemo, the dependency list already matches what’s referenced, so the disable looks unnecessary and should be removed to keep hooks lint trustworthy.
| const pinchZoomHandler = useMemo(() => { | ||
| return createPinchZoomHandler({ | ||
| getViewport, | ||
| setViewport, | ||
| setCenter, // fallback if setViewport unavailable | ||
| getContainer: () => containerRef.current, | ||
| minZoom: 0.2, | ||
| maxZoom: 2, | ||
| boost: 2.5, | ||
| }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [getViewport, setViewport, setCenter]); |
There was a problem hiding this comment.
The eslint disables are masking dependency issues rather than fixing them. For the focusNodeId effect, either include the real dependencies (or wrap focusNodeHandle in useCallback) so the closure is correct; for the useMemo, the dependency list already matches what’s referenced, so the disable looks unnecessary and should be removed to keep hooks lint trustworthy.
| }, [pinchZoomHandler]); | ||
|
|
||
| return ( | ||
| <div ref={containerRef} className="relative flex flex-col h-full flex-grow" style={{ touchAction: 'none' }}> |
There was a problem hiding this comment.
Setting touchAction: 'none' on the whole container disables native touch behaviors (including scrolling) within that region, which can negatively impact touch/trackpad users and assistive interactions. If the goal is only to control gestures on the canvas, consider scoping touchAction more narrowly (e.g., to the React Flow pane element) or using a less restrictive value (manipulation) where possible.
| <div ref={containerRef} className="relative flex flex-col h-full flex-grow" style={{ touchAction: 'none' }}> | |
| <div ref={containerRef} className="relative flex flex-col h-full flex-grow" style={{ touchAction: 'manipulation' }}> |
|
@Bibli2311 I'm unsure if this is handled in a decent way. From what I can see, it isnt. So i believe we should close this MR, and let it remain an issue for now. Could probably be handled in a much more concise way. |
pinch zoom
Added pinch zoom where zooming is faster when pressing CTRL while zooming.
Issues
These changes doesn't work as expected for Mac users, see this issue from React flow:
xyflow/xyflow#4416