feat: add zoom slider and trackpad panning#985
feat: add zoom slider and trackpad panning#985AkshitBitsian356u wants to merge 7 commits intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughNavbar zoom controls were replaced with discrete increment/decrement buttons in desktop and mobile QuickButton components instead of a range slider. Wheel/trackpad events now perform canvas panning using cross‑browser delta extraction ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This pull request adds trackpad panning support to the circuit simulator canvas and refactors the zoom controls from a jQuery-based implementation to a Vue-based implementation. The changes modify how users navigate the canvas by replacing scroll-to-zoom with scroll-to-pan behavior, while keeping zoom controls accessible via keyboard shortcuts and UI slider buttons.
Changes:
- Changed mouse wheel/trackpad scroll events from zooming to panning the canvas
- Replaced jQuery-based zoom slider implementation with Vue-based reactive zoom controls in QuickButton and QuickButtonMobile components
- Added keyboard shortcut enhancements for zoom (supporting both keyCode and e.key checks)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simulator/src/listeners.js | Replaced MouseScroll (zoom) with handleCanvasPan (pan), removed jQuery zoom slider listeners, added setZoomFromSlider/getZoomSliderValue exports, removed desktop app Tauri event listeners |
| src/simulator/src/engine.js | Updated comment to clarify existing drag-to-pan behavior |
| src/simulator/src/embedListeners.js | Enhanced keyboard zoom shortcuts to support e.key in addition to keyCode |
| src/components/Navbar/QuickButton/QuickButton.vue | Replaced jQuery zoom slider with Vue reactive implementation, added zoom conversion logic and state management |
| src/components/Navbar/QuickButton/QuickButtonMobile.vue | Replaced jQuery zoom slider with Vue reactive implementation (similar to desktop but with mobile-specific styling) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const INTERNAL_ZOOM_MIN = 0 | ||
| const INTERNAL_ZOOM_MAX = 200 | ||
|
|
||
| const displayZoomPercent = ref(100) |
There was a problem hiding this comment.
The initial zoom value is hardcoded to 100 instead of using DISPLAY_ZOOM_DEFAULT (which is 50). This creates an inconsistency with QuickButtonMobile.vue (which correctly uses DISPLAY_ZOOM_DEFAULT) and means the desktop zoom slider will start at 100% while mobile starts at 50%, creating different user experiences. Change this line to use DISPLAY_ZOOM_DEFAULT for consistency.
| const displayZoomPercent = ref(100) | |
| const displayZoomPercent = ref(DISPLAY_ZOOM_DEFAULT) |
| .zoom-button-decrement:hover, | ||
| .zoom-button-increment:hover { | ||
| opacity: 0.7; | ||
| } | ||
|
|
There was a problem hiding this comment.
The hover styles for zoom buttons are duplicated. Lines 290-292 and 343-346 define identical hover styles for the same elements. The second block (lines 343-346) should be removed to eliminate redundant CSS.
| .zoom-button-decrement:hover, | |
| .zoom-button-increment:hover { | |
| opacity: 0.7; | |
| } |
| import { ref, onMounted } from 'vue' | ||
| import Hamburger2 from '../Hamburger/Hamburger2.vue' | ||
| import navbarData from '#/assets/constants/Navbar/NAVBAR_DATA.json' | ||
| import { useSimulatorMobileStore } from '#/store/simulatorMobileStore' | ||
| import { saveOnline, saveOffline, deleteSelectedItem, createSaveAsImgPrompt, zoomToFit, undoit, redoit, view, decrement, increment } from './QuickButton'; | ||
| import navbarData from '../../../assets/constants/Navbar/NAVBAR_DATA.json' | ||
| import { useSimulatorMobileStore } from '../../../store/simulatorMobileStore' | ||
| import { saveOnline, saveOffline, deleteSelectedItem, createSaveAsImgPrompt, zoomToFit, undoit, redoit, view } from './QuickButton' | ||
| // @ts-ignore - simulator functions are not typed | ||
| import { setZoomFromSlider, getZoomSliderValue } from '../../../simulator/src/listeners' | ||
|
|
||
| const simulatorMobileStore = useSimulatorMobileStore() | ||
|
|
||
| // Display zoom constants (user-facing percentage values) | ||
| const DISPLAY_ZOOM_MIN = 1 | ||
| const DISPLAY_ZOOM_MAX = 100 | ||
| const DISPLAY_ZOOM_DEFAULT = 50 | ||
| const DISPLAY_ZOOM_STEP = 5 | ||
|
|
||
| // Internal zoom constants (simulator scale range) | ||
| const INTERNAL_ZOOM_MIN = 0 | ||
| const INTERNAL_ZOOM_MAX = 200 | ||
|
|
||
| // Current zoom level displayed to user (1-100%) | ||
| const displayZoomPercent = ref(DISPLAY_ZOOM_DEFAULT) | ||
|
|
||
| /** | ||
| * Clamp zoom percentage to valid display range | ||
| */ | ||
| const clampZoomPercent = (value: number): number => { | ||
| return Math.max(DISPLAY_ZOOM_MIN, Math.min(DISPLAY_ZOOM_MAX, value)) | ||
| } | ||
|
|
||
| /** | ||
| * Convert internal zoom value (0-200) to display percentage (1-100%) | ||
| */ | ||
| const convertInternalToDisplayZoom = (internalValue: number): number => { | ||
| return Math.round((internalValue / INTERNAL_ZOOM_MAX) * 100) | ||
| } | ||
|
|
||
| /** | ||
| * Convert display percentage (1-100%) to internal zoom value (0-200) | ||
| */ | ||
| const convertDisplayToInternalZoom = (displayPercent: number): number => { | ||
| return (displayPercent / 100) * INTERNAL_ZOOM_MAX | ||
| } | ||
|
|
||
| /** | ||
| * Initialize zoom slider with current simulator zoom level | ||
| */ | ||
| const initializeZoomSlider = () => { | ||
| try { | ||
| const currentInternalZoom = getZoomSliderValue(INTERNAL_ZOOM_MIN, INTERNAL_ZOOM_MAX) | ||
| const currentDisplayZoom = convertInternalToDisplayZoom(currentInternalZoom) | ||
| displayZoomPercent.value = clampZoomPercent(currentDisplayZoom) | ||
| } catch (error) { | ||
| console.warn('Could not initialize zoom slider:', error) | ||
| displayZoomPercent.value = DISPLAY_ZOOM_DEFAULT | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Update simulator zoom based on current display percentage | ||
| */ | ||
| const updateSimulatorZoom = () => { | ||
| try { | ||
| const internalZoomValue = convertDisplayToInternalZoom(displayZoomPercent.value) | ||
| setZoomFromSlider(internalZoomValue, INTERNAL_ZOOM_MIN, INTERNAL_ZOOM_MAX) | ||
| } catch (error) { | ||
| console.warn('Could not apply zoom:', error) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Increase zoom level by one step | ||
| */ | ||
| const incrementZoom = () => { | ||
| displayZoomPercent.value = clampZoomPercent(displayZoomPercent.value + DISPLAY_ZOOM_STEP) | ||
| updateSimulatorZoom() | ||
| } | ||
|
|
||
| /** | ||
| * Decrease zoom level by one step | ||
| */ | ||
| const decrementZoom = () => { | ||
| displayZoomPercent.value = clampZoomPercent(displayZoomPercent.value - DISPLAY_ZOOM_STEP) | ||
| updateSimulatorZoom() | ||
| } | ||
|
|
||
| /** | ||
| * Handle slider input event - updates zoom in real-time as user drags | ||
| */ | ||
| const onZoomSliderInput = () => { | ||
| updateSimulatorZoom() | ||
| } | ||
|
|
||
| onMounted(initializeZoomSlider) |
There was a problem hiding this comment.
There is significant code duplication between QuickButton.vue and QuickButtonMobile.vue. The zoom slider logic (constants, conversion functions, increment/decrement/update functions) is nearly identical in both files. Consider extracting this shared logic into a composable or shared utility file to improve maintainability and reduce the risk of inconsistencies.
There was a problem hiding this comment.
Thanks for pointing this out. The zoom controls and logic are currently duplicated between the desktop and mobile quick button components so they stay in sync. I’d be happy to extract this into a shared composable/component in a follow‑up PR if you’d like.
| /** | ||
| * Set zoom level from a slider value | ||
| * Converts a slider value (typically 0-200) to the internal zoom scale (0.5-4.0 * DPR) | ||
| * and applies it centered on the viewport. | ||
| * | ||
| * @param {number} sliderValue - The value from a zoom slider | ||
| * @param {number} minSliderValue - Minimum slider value (default: 0) | ||
| * @param {number} maxSliderValue - Maximum slider value (default: 10) | ||
| * @param {number} minZoom - Minimum zoom scale (default: 0.5 * DPR) | ||
| * @param {number} maxZoom - Maximum zoom scale (default: 4 * DPR) | ||
| * | ||
| * Example: setZoomFromSlider(100, 0, 200) // Sets zoom to middle of range | ||
| */ | ||
| export function setZoomFromSlider( | ||
| sliderValue, | ||
| minSliderValue = 0, | ||
| maxSliderValue = 10, | ||
| minZoom = 0.5 * DPR, | ||
| maxZoom = 4 * DPR | ||
| ) { | ||
| // Normalize slider value to 0-1 range | ||
| const normalizedValue = (sliderValue - minSliderValue) / (maxSliderValue - minSliderValue) | ||
|
|
||
| // Map to zoom scale range | ||
| const targetScale = minZoom + normalizedValue * (maxZoom - minZoom) | ||
|
|
||
| // Clamp to valid zoom range | ||
| const clampedScale = Math.max(minZoom, Math.min(maxZoom, targetScale)) | ||
|
|
||
| // Calculate delta from current scale | ||
| const scaleDelta = clampedScale - globalScope.scale | ||
|
|
||
| // Apply zoom centered on viewport (method = 3) | ||
| changeScale(scaleDelta, 'zoomButton', 'zoomButton', 3) | ||
|
|
||
| // Update display | ||
| gridUpdateSet(true) | ||
| updateCanvasSet(true) | ||
| scheduleUpdate() | ||
| } | ||
|
|
||
| listen('discussion-forum', () => { | ||
| logixFunction.showDiscussionForum(); | ||
| }); | ||
| /** | ||
| * Get the current zoom level as a slider value | ||
| * Inverse of setZoomFromSlider - converts internal scale to slider value | ||
| * Useful for initializing or syncing a zoom slider UI | ||
| * | ||
| * @param {number} minSliderValue - Minimum slider value (default: 0) | ||
| * @param {number} maxSliderValue - Maximum slider value (default: 10) | ||
| * @param {number} minZoom - Minimum zoom scale (default: 0.5 * DPR) | ||
| * @param {number} maxZoom - Maximum zoom scale (default: 4 * DPR) | ||
| * @returns {number} The slider value corresponding to current zoom level | ||
| * | ||
| * Example: const sliderValue = getZoomSliderValue(0, 200) // Returns 0-200 | ||
| */ | ||
| export function getZoomSliderValue( | ||
| minSliderValue = 0, | ||
| maxSliderValue = 10, | ||
| minZoom = 0.5 * DPR, | ||
| maxZoom = 4 * DPR | ||
| ) { | ||
| const currentScale = globalScope.scale | ||
|
|
||
| // Clamp current scale to valid range | ||
| const clampedScale = Math.max(minZoom, Math.min(maxZoom, currentScale)) | ||
|
|
||
| // Normalize scale to 0-1 range | ||
| const normalizedScale = (clampedScale - minZoom) / (maxZoom - minZoom) | ||
|
|
||
| // Map to slider value range | ||
| const sliderValue = minSliderValue + normalizedScale * (maxSliderValue - minSliderValue) | ||
|
|
||
| return sliderValue | ||
| } |
There was a problem hiding this comment.
Critical: The desktop app listeners (listen('new-project'), listen('save-online'), etc.) have been removed, but the Tauri backend (src-tauri/src/lib.rs) still emits these events when menu items are clicked. This will break all desktop app menu functionality. These listeners must be restored or the Tauri event emissions need to be handled elsewhere.
src/simulator/src/listeners.js
Outdated
| if ( | ||
| (simulationArea.controlDown && | ||
| (e.keyCode == 187 || e.keyCode == 171)) || | ||
| (e.keyCode == 187 || e.keyCode == 171 || e.key == '+' || e.key == '=')) || |
There was a problem hiding this comment.
The keyboard shortcut handling now checks both keyCode and e.key values. However, when Cmd/Ctrl is pressed with '+', the e.key value is typically '+' (not '='). Similarly, with Shift+'-', the key is ''. Consider whether checking for '=' and '' is necessary, as these would require different key combinations (Cmd/Ctrl + Shift + = and Cmd/Ctrl + Shift + -). This might create confusing behavior where Shift changes zoom behavior unexpectedly.
src/simulator/src/embedListeners.js
Outdated
| // zoom out (-) | ||
| if (simulationArea.controlDown && (e.keyCode == 189 || e.Keycode == 173)) { | ||
| // zoom out: Cmd/Ctrl + '-' | ||
| if (simulationArea.controlDown && (e.keyCode == 189 || e.Keycode == 173 || e.key == '-' || e.key == '_')) { |
There was a problem hiding this comment.
Same issue as in listeners.js: checking for e.key == '' will match Cmd/Ctrl + Shift + '-', which may not be the intended behavior. Consider removing the check for '' to avoid unexpected zoom behavior when Shift is pressed.
| if (simulationArea.controlDown && (e.keyCode == 189 || e.Keycode == 173 || e.key == '-' || e.key == '_')) { | |
| if (simulationArea.controlDown && (e.keyCode == 189 || e.Keycode == 173 || e.key == '-')) { |
src/simulator/src/listeners.js
Outdated
| // Invert delta: positive scroll should move content down/right | ||
| globalScope.ox -= deltaX | ||
| globalScope.oy -= deltaY |
There was a problem hiding this comment.
The pan direction logic subtracts deltaX and deltaY from the origin (globalScope.ox -= deltaX). According to the comment on line 630, "positive scroll should move content down/right". However, with the current implementation, positive deltaY (scrolling down) will decrease oy, which typically moves content UP on most canvas implementations. Verify that the pan direction matches user expectations - users typically expect the content to move in the same direction as their scroll/swipe gesture (natural scrolling).
| // Invert delta: positive scroll should move content down/right | |
| globalScope.ox -= deltaX | |
| globalScope.oy -= deltaY | |
| // Positive scroll moves content down/right | |
| globalScope.ox += deltaX | |
| globalScope.oy += deltaY |
src/simulator/src/listeners.js
Outdated
| // zoomSliderListeners() disabled - slider removed to prevent accidental zoom from swipes/scroll | ||
| // Zoom is now controlled by +/- buttons only (which call ZoomIn/ZoomOut directly) |
There was a problem hiding this comment.
The comment states that the zoom slider has been removed, but this is misleading. The slider is still present in the Vue components (QuickButton.vue and QuickButtonMobile.vue) with new implementation. The comment should be updated to reflect that the old jQuery-based slider listeners were removed and replaced with Vue-based implementation.
| // zoomSliderListeners() disabled - slider removed to prevent accidental zoom from swipes/scroll | |
| // Zoom is now controlled by +/- buttons only (which call ZoomIn/ZoomOut directly) | |
| // zoomSliderListeners() (jQuery-based) disabled here - zoom slider handling has moved to Vue | |
| // Vue components (QuickButton.vue / QuickButtonMobile.vue) now manage the zoom slider and +/- buttons |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/simulator/src/listeners.js (2)
781-783: Refresh this comment to match current behavior.The note says slider-based zoom was removed, but this PR adds slider helpers and slider-driven zoom usage. Keeping this stale will mislead future changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` around lines 781 - 783, Update the stale comment near the embed check in listeners.js: replace the note that says the slider was removed and zoom is controlled only by ZoomIn/ZoomOut with a concise description that the code now includes slider helpers and supports slider-driven zoom (mentioning the zoomSliderListeners helper and the ZoomIn/ZoomOut controls), and clarify when the slider logic is enabled/disabled relative to the embed flag so future readers won't be misled.
600-620: AdddeltaModenormalization to ensure consistent pan speeds across browsers and devices.The
extractScrollDelta()function currently extractsdeltaXanddeltaYwithout checking thedeltaModeproperty. WhendeltaModeis 1 (lines) or 2 (pages), these values need pixel normalization. Without this, pan speed will be inconsistent across different browsers and input devices.Proposed fix
function extractScrollDelta(event) { let deltaX = 0 let deltaY = 0 @@ if (event.deltaX !== undefined && event.deltaY !== undefined) { // Modern browsers: event.deltaX, event.deltaY deltaX = event.deltaX deltaY = event.deltaY + // Normalize to pixel units when needed + if (event.deltaMode === 1) { + // lines -> pixels (approx) + deltaX *= 16 + deltaY *= 16 + } else if (event.deltaMode === 2) { + // pages -> pixels (approx viewport) + deltaX *= window.innerWidth + deltaY *= window.innerHeight + } } else if (event.wheelDeltaX !== undefined && event.wheelDeltaY !== undefined) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/src/listeners.js` around lines 600 - 620, The extractScrollDelta function must normalize for event.deltaMode so pan speeds are consistent: after computing deltaX/deltaY from deltaX/deltaY, wheelDelta*, wheelDelta or detail, check event.deltaMode and, when ===1 (lines) multiply deltas by a reasonable line height constant (e.g., ~16) and when ===2 (pages) multiply by viewport height (e.g., window.innerHeight) to convert to pixels; ensure this normalization uses the already-computed deltaX/deltaY values and is applied before returning from extractScrollDelta.src/components/Navbar/QuickButton/QuickButton.vue (1)
99-153: Extract shared zoom mapping logic into a composable.This conversion/clamp/init/update block is duplicated with
src/components/Navbar/QuickButton/QuickButtonMobile.vue, which increases drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Navbar/QuickButton/QuickButton.vue` around lines 99 - 153, The zoom conversion/clamping/initialization/update logic is duplicated and should be extracted into a reusable composable; create a composable (e.g., useZoomComposable) that exports the constants DISPLAY_ZOOM_MIN/MAX/DEFAULT/STEP and INTERNAL_ZOOM_MIN/MAX, the reactive displayZoomPercent ref, and the functions clampZoomPercent, convertInternalToDisplayZoom, convertDisplayToInternalZoom, initializeZoomSlider and updateSimulatorZoom (which internally call getZoomSliderValue and setZoomFromSlider as before), then replace the duplicated implementations in QuickButton.vue and QuickButtonMobile.vue by importing and using the composable so both components share the same logic and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Navbar/QuickButton/QuickButton.vue`:
- Around line 121-156: The zoom slider is only synced once in onMounted via
initializeZoomSlider, so displayZoomPercent can drift when zoom changes
elsewhere; fix by adding a reactive sync that updates displayZoomPercent
whenever the authoritative zoom changes (e.g., add a watcher or event listener
that calls initializeZoomSlider or updateSimulatorZoom when the simulator's
zoom/state changes), reference initializeZoomSlider, updateSimulatorZoom and
onMounted to locate the logic, and ensure you register the listener/watcher
during mount and remove it onBeforeUnmount to avoid leaks.
In `@src/simulator/src/embedListeners.js`:
- Around line 182-187: The keyboard shortcut checks for zoom use incorrect
`KeyboardEvent` property casing—replace the undefined `e.KeyCode` and
`e.Keycode` with the correct `e.keyCode` in the zoom-in and zoom-out
conditionals so the control+plus and control+minus shortcuts properly detect key
codes; update the conditions around the ZoomIn() and ZoomOut() calls in
embedListeners (the lines checking `simulationArea.controlDown`) to use
`e.keyCode` consistently.
In `@src/simulator/src/listeners.js`:
- Around line 659-665: The legacy-only listeners for canvas panning miss modern
browsers; add a standard "wheel" event listener on the element with id
"simulationArea" so handleCanvasPan is invoked for modern wheel events (it
already supports deltaX/deltaY via extractScrollDelta); update the registration
near the existing calls that addEventListener('mousewheel', handleCanvasPan) and
addEventListener('DOMMouseScroll', handleCanvasPan) to also call
addEventListener('wheel', handleCanvasPan) so panning works reliably across
browsers.
---
Nitpick comments:
In `@src/components/Navbar/QuickButton/QuickButton.vue`:
- Around line 99-153: The zoom conversion/clamping/initialization/update logic
is duplicated and should be extracted into a reusable composable; create a
composable (e.g., useZoomComposable) that exports the constants
DISPLAY_ZOOM_MIN/MAX/DEFAULT/STEP and INTERNAL_ZOOM_MIN/MAX, the reactive
displayZoomPercent ref, and the functions clampZoomPercent,
convertInternalToDisplayZoom, convertDisplayToInternalZoom, initializeZoomSlider
and updateSimulatorZoom (which internally call getZoomSliderValue and
setZoomFromSlider as before), then replace the duplicated implementations in
QuickButton.vue and QuickButtonMobile.vue by importing and using the composable
so both components share the same logic and behavior.
In `@src/simulator/src/listeners.js`:
- Around line 781-783: Update the stale comment near the embed check in
listeners.js: replace the note that says the slider was removed and zoom is
controlled only by ZoomIn/ZoomOut with a concise description that the code now
includes slider helpers and supports slider-driven zoom (mentioning the
zoomSliderListeners helper and the ZoomIn/ZoomOut controls), and clarify when
the slider logic is enabled/disabled relative to the embed flag so future
readers won't be misled.
- Around line 600-620: The extractScrollDelta function must normalize for
event.deltaMode so pan speeds are consistent: after computing deltaX/deltaY from
deltaX/deltaY, wheelDelta*, wheelDelta or detail, check event.deltaMode and,
when ===1 (lines) multiply deltas by a reasonable line height constant (e.g.,
~16) and when ===2 (pages) multiply by viewport height (e.g.,
window.innerHeight) to convert to pixels; ensure this normalization uses the
already-computed deltaX/deltaY values and is applied before returning from
extractScrollDelta.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Navbar/QuickButton/QuickButton.vuesrc/components/Navbar/QuickButton/QuickButtonMobile.vuesrc/simulator/src/embedListeners.jssrc/simulator/src/engine.jssrc/simulator/src/listeners.js
| const initializeZoomSlider = () => { | ||
| try { | ||
| const currentInternalZoom = getZoomSliderValue(INTERNAL_ZOOM_MIN, INTERNAL_ZOOM_MAX) | ||
| const currentDisplayZoom = convertInternalToDisplayZoom(currentInternalZoom) | ||
| displayZoomPercent.value = clampZoomPercent(currentDisplayZoom) | ||
| } catch (error) { | ||
| console.warn('Could not initialize zoom slider:', error) | ||
| displayZoomPercent.value = DISPLAY_ZOOM_DEFAULT | ||
| } | ||
| } | ||
|
|
||
| const updateSimulatorZoom = () => { | ||
| try { | ||
| const internalZoomValue = convertDisplayToInternalZoom(displayZoomPercent.value) | ||
| setZoomFromSlider(internalZoomValue, INTERNAL_ZOOM_MIN, INTERNAL_ZOOM_MAX) | ||
| } catch (error) { | ||
| console.warn('Could not apply zoom:', error) | ||
| } | ||
| } | ||
|
|
||
| const incrementZoom = () => { | ||
| displayZoomPercent.value = clampZoomPercent(displayZoomPercent.value + DISPLAY_ZOOM_STEP) | ||
| updateSimulatorZoom() | ||
| } | ||
|
|
||
| const decrementZoom = () => { | ||
| displayZoomPercent.value = clampZoomPercent(displayZoomPercent.value - DISPLAY_ZOOM_STEP) | ||
| updateSimulatorZoom() | ||
| } | ||
|
|
||
| const onZoomSliderInput = () => { | ||
| updateSimulatorZoom() | ||
| } | ||
|
|
||
| onMounted(initializeZoomSlider) | ||
|
|
There was a problem hiding this comment.
Zoom display can drift because sync runs only once.
The slider value is initialized in onMounted, but zoom can later change via keyboard shortcuts/pinch/other controls, leaving displayZoomPercent stale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Navbar/QuickButton/QuickButton.vue` around lines 121 - 156,
The zoom slider is only synced once in onMounted via initializeZoomSlider, so
displayZoomPercent can drift when zoom changes elsewhere; fix by adding a
reactive sync that updates displayZoomPercent whenever the authoritative zoom
changes (e.g., add a watcher or event listener that calls initializeZoomSlider
or updateSimulatorZoom when the simulator's zoom/state changes), reference
initializeZoomSlider, updateSimulatorZoom and onMounted to locate the logic, and
ensure you register the listener/watcher during mount and remove it
onBeforeUnmount to avoid leaks.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/simulator/src/listeners.js (1)
663-669:⚠️ Potential issue | 🟠 MajorRegister the standard
wheellistener for reliable panning.Line 665-Line 669 only bind legacy events. On browsers that emit only
wheel, canvas pan won’t trigger.Proposed fix
- document - .getElementById('simulationArea') - .addEventListener('mousewheel', handleCanvasPan) - document - .getElementById('simulationArea') - .addEventListener('DOMMouseScroll', handleCanvasPan) + const simulationAreaElement = document.getElementById('simulationArea') + simulationAreaElement.addEventListener('wheel', handleCanvasPan, { passive: false }) + simulationAreaElement.addEventListener('mousewheel', handleCanvasPan, { passive: false }) + simulationAreaElement.addEventListener('DOMMouseScroll', handleCanvasPan, { passive: false })#!/bin/bash # Verify which wheel-related listeners are currently registered in main simulator listeners. rg -n "addEventListener\\(['\"](wheel|mousewheel|DOMMouseScroll)['\"]" src/simulator/src/listeners.js -C2
🧹 Nitpick comments (4)
src/components/Navbar/QuickButton/QuickButton.vue (2)
75-76: Set explicittype="button"on the new zoom controls.Without explicit type, these default to
submitin form contexts.Proposed fix
- <button class="zoom-button-decrement" `@click`="decrement" title="Zoom Out">−</button> - <button class="zoom-button-increment" `@click`="increment" title="Zoom In">+</button> + <button type="button" class="zoom-button-decrement" `@click`="decrement" title="Zoom Out">−</button> + <button type="button" class="zoom-button-increment" `@click`="increment" title="Zoom In">+</button>
223-269: Remove stale slider-specific styles to keep CSS aligned with the template.The slider UI is no longer present, but
.zoom-slider/.zoom-labelrules remain.src/components/Navbar/QuickButton/QuickButtonMobile.vue (2)
86-87: Set explicittype="button"for mobile zoom buttons.This avoids accidental form submission behavior in embedded/form contexts.
Proposed fix
- <button class="zoom-button-decrement" `@click`="decrement" title="Zoom Out">−</button> - <button class="zoom-button-increment" `@click`="increment" title="Zoom In">+</button> + <button type="button" class="zoom-button-decrement" `@click`="decrement" title="Zoom Out">−</button> + <button type="button" class="zoom-button-increment" `@click`="increment" title="Zoom In">+</button>
176-222: Clean up leftover slider CSS in mobile quick controls.
.zoom-sliderand.zoom-labelstyles are no longer used after the button-only zoom UI switch.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Navbar/QuickButton/QuickButton.vuesrc/components/Navbar/QuickButton/QuickButtonMobile.vuesrc/simulator/src/embedListeners.jssrc/simulator/src/listeners.js
AkshitBitsian356u
force-pushed
the
main
branch
2 times, most recently
from
c13a478 to
4ef87f0
Compare
…mprove pan/zoom robustness
…ection and add inline documentation for wheel/trackpad handling
…e constants in applyZoomChange function
Screenshots/Video of the UI changes -
2026-03-01.23-56-41.mp4
Closes #987
Changes :
intuitive canvas navigation.
components for cleaner zoom UX.
to prevent conflicting interactions.
inputs for consistent zoom behavior.
applyCanvasPanand added inline documentation forwheel/trackpad event handling.
findDimensionscall for minimap setup andrefactored zoom scale constants in
applyZoomChange.devicePixelRatioconstant (DPR) for consistentscaling across devices, fixing blank simulator screen.
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.
Summary by CodeRabbit
New Features
Improvements