fix(ui): prevent ElementsPanel from being dragged under Navbar (#260) #691
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe PR introduces navbar overlap prevention for draggable panels across the simulator codebase. A new test suite validates drag behavior constraints relative to a fixed navbar, while drag logic in multiple implementation files is modified to compute positions before application and clamp vertical positioning to prevent overlapping the navbar's bottom edge. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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)
v1/src/simulator/src/drag.ts (1)
29-46: Cache the navbar reference to avoid repeated DOM queries.The logic correctly prevents navbar overlap and handles the missing navbar case gracefully. However,
document.querySelector('.navbar.header')is called on everymoveevent, which could impact performance during intensive dragging.Consider caching the navbar reference at initialization time in the
dragging()function and passing it toupdatePosition():function updatePosition( element: HTMLElement, dx: number, dy: number, - positions: WeakMap<HTMLElement, Position> + positions: WeakMap<HTMLElement, Position>, + navbar: HTMLElement | null ): void { if (!element) return if (!positions.has(element)) { positions.set(element, { x: 0, y: 0 }) } const currentPosition = positions.get(element) if (!currentPosition) return let newX = currentPosition.x + dx let newY = currentPosition.y + dy - const navbar = document.querySelector('.navbar.header') as HTMLElement if (navbar) { const navbarRect = navbar.getBoundingClientRect() // ... rest of logic } currentPosition.x = newX currentPosition.y = newY element.style.transform = `translate(${currentPosition.x}px, ${currentPosition.y}px)` }Then in the
dragging()function:export function dragging(targetEl: HTMLElement, DragEl: HTMLElement): void { const positions = new WeakMap<HTMLElement, Position>() + const navbar = document.querySelector('.navbar.header') as HTMLElement | null interact(DragEl).draggable({ allowFrom: targetEl, listeners: { move(event) { updatePosition( event.target as HTMLElement, event.dx, event.dy, - positions + positions, + navbar ) }, }, // ... }) }src/simulator/src/drag.ts (1)
25-49: Duplicate implementation across versions - consider shared module.This file contains identical changes to
v1/src/simulator/src/drag.tsandv0/src/simulator/src/drag.ts. The same performance optimization suggested for the navbar querySelector caching applies here.If these three versions (src, v0, v1) are maintained separately, ensure that any future fixes or optimizations are consistently applied across all versions.
v0/src/simulator/src/drag.ts (1)
25-49: Duplicate implementation - same concerns as other versions.This is the third duplicate of the navbar collision detection logic. The querySelector performance optimization and code duplication concerns noted in the reviews of
v1/src/simulator/src/drag.tsandsrc/simulator/src/drag.tsapply here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/simulator/spec/drag.spec.js(1 hunks)src/simulator/src/drag.ts(1 hunks)v0/src/simulator/src/drag.ts(1 hunks)v1/src/simulator/src/drag.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/simulator/spec/drag.spec.js (1)
src/simulator/src/drag.ts (1)
dragging(70-111)
v0/src/simulator/src/drag.ts (1)
src/simulator/src/ux.js (1)
element(484-484)
🔇 Additional comments (4)
v1/src/simulator/src/drag.ts (2)
25-27: LGTM: Two-step position calculation enables clamping logic.The refactoring to calculate
newXandnewYbefore updatingcurrentPositionis a clean approach that enables the navbar collision detection to adjust the position before it's applied.
48-49: LGTM: Position update correctly applies clamped values.The final position update correctly uses the potentially adjusted
newXandnewYvalues, ensuring the navbar constraint is enforced when the transform is applied.src/simulator/spec/drag.spec.js (2)
1-37: LGTM: Test setup is well-structured.The test suite setup properly creates mock DOM elements with appropriate classes and styles. The
beforeEachhook ensures clean state for each test.
103-111: LGTM: Properly tests graceful degradation.This test correctly verifies that dragging initializes without error when the navbar is absent, ensuring the graceful fallback behavior works as intended.
| test('should prevent panel from overlapping navbar when dragged upward', () => { | ||
| // Mock getBoundingClientRect for navbar | ||
| const navbarGetBoundingClientRect = vi.fn(() => ({ | ||
| top: 0, | ||
| bottom: 60, | ||
| left: 0, | ||
| right: window.innerWidth, | ||
| width: window.innerWidth, | ||
| height: 60, | ||
| })); | ||
| mockNavbar.getBoundingClientRect = navbarGetBoundingClientRect; | ||
|
|
||
| // Mock getBoundingClientRect for panel (initial position) | ||
| const panelGetBoundingClientRect = vi.fn(() => ({ | ||
| top: 100, | ||
| bottom: 400, | ||
| left: 50, | ||
| right: 250, | ||
| width: 200, | ||
| height: 300, | ||
| })); | ||
| mockPanel.getBoundingClientRect = panelGetBoundingClientRect; | ||
|
|
||
| // Initialize dragging | ||
| dragging(mockHeader, mockPanel); | ||
|
|
||
| // Simulate manual position update that would overlap navbar | ||
| // This simulates what happens when a panel is dragged upward | ||
| const transform = mockPanel.style.transform; | ||
|
|
||
| // The panel should not be able to move above navbar bottom (60px) | ||
| // If initial top is 100px and navbar bottom is 60px, | ||
| // the minimum transform Y should keep panel at or below 60px | ||
| expect(transform).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen test: verify actual clamping behavior during drag events.
This test only verifies that dragging() initializes successfully and that a transform is defined. It doesn't actually trigger drag move events or verify that the navbar collision detection works correctly.
The mocked getBoundingClientRect functions are never called because no drag events are simulated. Consider:
- Export
updatePositionfrom drag.ts to enable direct testing of the collision logic:
// In drag.ts
export function updatePosition(/*...*/) { /*...*/ }- Test the actual clamping behavior:
test('should prevent panel from overlapping navbar when dragged upward', () => {
// Setup mocks as before
mockNavbar.getBoundingClientRect = vi.fn(() => ({
top: 0, bottom: 60, /*...*/
}));
mockPanel.getBoundingClientRect = vi.fn(() => ({
top: 100, bottom: 400, /*...*/
}));
const positions = new WeakMap();
positions.set(mockPanel, { x: 0, y: 0 });
// Simulate dragging upward by 50px (would overlap navbar at y=60)
updatePosition(mockPanel, 0, -50, positions);
// Verify clamping occurred
const transform = mockPanel.style.transform;
expect(transform).toMatch(/translate\(0px, -40px\)/); // Clamped to navbar bottom
expect(mockNavbar.getBoundingClientRect).toHaveBeenCalled();
});Alternatively, if updatePosition should remain private, use interact.js's testing utilities to simulate drag events.
| test('should allow panel to be dragged freely when not near navbar', () => { | ||
| // Mock getBoundingClientRect for navbar | ||
| mockNavbar.getBoundingClientRect = vi.fn(() => ({ | ||
| top: 0, | ||
| bottom: 60, | ||
| left: 0, | ||
| right: window.innerWidth, | ||
| width: window.innerWidth, | ||
| height: 60, | ||
| })); | ||
|
|
||
| // Mock getBoundingClientRect for panel (far from navbar) | ||
| mockPanel.getBoundingClientRect = vi.fn(() => ({ | ||
| top: 200, | ||
| bottom: 500, | ||
| left: 50, | ||
| right: 250, | ||
| width: 200, | ||
| height: 300, | ||
| })); | ||
|
|
||
| // Initialize dragging | ||
| dragging(mockHeader, mockPanel); | ||
|
|
||
| // Panel should be able to move freely when far from navbar | ||
| expect(mockPanel.style.transform).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test doesn't verify free dragging behavior.
Similar to the previous test, this only verifies initialization. It should simulate a drag event and verify that the panel can move freely without being clamped when far from the navbar.
| test('should use runtime navbar height via getBoundingClientRect', () => { | ||
| const getBoundingClientRectSpy = vi.fn(() => ({ | ||
| top: 0, | ||
| bottom: 75, // Dynamic height | ||
| left: 0, | ||
| right: window.innerWidth, | ||
| width: window.innerWidth, | ||
| height: 75, | ||
| })); | ||
|
|
||
| mockNavbar.getBoundingClientRect = getBoundingClientRectSpy; | ||
|
|
||
| // Initialize dragging | ||
| dragging(mockHeader, mockPanel); | ||
|
|
||
| // The navbar's getBoundingClientRect should be callable | ||
| // (actual constraint logic is tested during drag interactions) | ||
| expect(getBoundingClientRectSpy).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test doesn't verify getBoundingClientRect is called.
The test creates a spy but doesn't simulate a drag event that would trigger the navbar collision detection logic. The assertion expect(getBoundingClientRectSpy).toBeDefined() is trivial since we just defined it.
Update the test to verify the spy is actually called:
test('should use runtime navbar height via getBoundingClientRect', () => {
const getBoundingClientRectSpy = vi.fn(() => ({
top: 0, bottom: 75, /*...*/
}));
mockNavbar.getBoundingClientRect = getBoundingClientRectSpy;
mockPanel.getBoundingClientRect = vi.fn(() => ({
top: 100, bottom: 400, /*...*/
}));
const positions = new WeakMap();
positions.set(mockPanel, { x: 0, y: 0 });
// Trigger updatePosition to invoke the spy
updatePosition(mockPanel, 0, -10, positions);
// Verify the spy was called
expect(getBoundingClientRectSpy).toHaveBeenCalled();
});🤖 Prompt for AI Agents
In src/simulator/spec/drag.spec.js around lines 113-131, the test only defines
getBoundingClientRectSpy and asserts it exists, which is trivial; update the
test to actually trigger the navbar collision logic so the spy is invoked and
then assert it was called. Specifically, assign mockNavbar.getBoundingClientRect
= getBoundingClientRectSpy, provide mockPanel.getBoundingClientRect (returning a
panel rect), create a positions WeakMap with an entry for mockPanel, call the
function that computes collisions (e.g., updatePosition or simulate a small drag
by invoking the same API used by dragging logic) to force the navbar rect
lookup, and replace the trivial expect(...).toBeDefined() with
expect(getBoundingClientRectSpy).toHaveBeenCalled().
1 participant
Fix: Prevent ElementsPanel from overlapping Navbar when dragged (#260)
Description
This PR fixes the issue where the ElementsPanel (and other draggable panels) could be dragged over the Navbar, causing visual overlap and poor UX.
Changes Made:
The fix works by:
Testing
navbar.overlapping.mp4
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.