Skip to content

fix(ui): prevent ElementsPanel from being dragged under Navbar (#260) #691

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
akritah wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ui): prevent ElementsPanel from being dragged under Navbar (#260) #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 132 additions & 0 deletions src/simulator/spec/drag.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
import { describe, test, expect, beforeEach, vi } from 'vitest';
import { dragging } from '../src/drag';

describe('Panel Dragging - Navbar Overlap Prevention', () => {
let mockPanel;
let mockHeader;
let mockNavbar;

beforeEach(() => {
// Reset document body
document.body.innerHTML = '';

// Create mock navbar
mockNavbar = document.createElement('nav');
mockNavbar.className = 'navbar header';
mockNavbar.style.position = 'fixed';
mockNavbar.style.top = '0';
mockNavbar.style.left = '0';
mockNavbar.style.width = '100%';
mockNavbar.style.height = '60px';
document.body.appendChild(mockNavbar);

// Create mock panel
mockPanel = document.createElement('div');
mockPanel.className = 'elementPanel draggable-panel';
mockPanel.style.position = 'absolute';
mockPanel.style.top = '100px';
mockPanel.style.left = '50px';
mockPanel.style.width = '200px';
mockPanel.style.height = '300px';
document.body.appendChild(mockPanel);

// Create mock panel header
mockHeader = document.createElement('div');
mockHeader.className = 'panel-header';
mockPanel.appendChild(mockHeader);
});

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();
});
Comment on lines +39 to +73
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Export updatePosition from drag.ts to enable direct testing of the collision logic:
// In drag.ts
export function updatePosition(/*...*/) { /*...*/ }
  1. 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();
});
Comment on lines +75 to +101
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 handle case when navbar does not exist', () => {
// Remove navbar
document.body.removeChild(mockNavbar);

// Initialize dragging without navbar present
expect(() => {
dragging(mockHeader, mockPanel);
}).not.toThrow();
});

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();
});
Comment on lines +113 to +131
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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().

});
28 changes: 26 additions & 2 deletions src/simulator/src/drag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,32 @@ function updatePosition(
// Update the element's x and y position
const currentPosition = positions.get(element)
if (!currentPosition) return // Check if the currentPosition is valid
currentPosition.x += dx
currentPosition.y += dy

// Calculate new position
let newX = currentPosition.x + dx
let newY = currentPosition.y + dy

// Get navbar bottom position to prevent overlap
const navbar = document.querySelector('.navbar.header') as HTMLElement
if (navbar) {
const navbarRect = navbar.getBoundingClientRect()
const navbarBottom = navbarRect.bottom

// Get element's current position on screen
const elementRect = element.getBoundingClientRect()
const elementTop = elementRect.top

// Calculate what the new top position would be after applying transform
const newElementTop = elementTop - currentPosition.y + newY

// Prevent panel from going above the navbar
if (newElementTop < navbarBottom) {
newY = currentPosition.y + (navbarBottom - elementTop)
}
}

currentPosition.x = newX
currentPosition.y = newY

// Apply the new position to the element using the CSS transform property
element.style.transform = `translate(${currentPosition.x}px, ${currentPosition.y}px)`
Expand Down
28 changes: 26 additions & 2 deletions v0/src/simulator/src/drag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,32 @@ function updatePosition(
// Update the element's x and y position
const currentPosition = positions.get(element)
if (!currentPosition) return // Check if the currentPosition is valid
currentPosition.x += dx
currentPosition.y += dy

// Calculate new position
let newX = currentPosition.x + dx
let newY = currentPosition.y + dy

// Get navbar bottom position to prevent overlap
const navbar = document.querySelector('.navbar.header') as HTMLElement
if (navbar) {
const navbarRect = navbar.getBoundingClientRect()
const navbarBottom = navbarRect.bottom

// Get element's current position on screen
const elementRect = element.getBoundingClientRect()
const elementTop = elementRect.top

// Calculate what the new top position would be after applying transform
const newElementTop = elementTop - currentPosition.y + newY

// Prevent panel from going above the navbar
if (newElementTop < navbarBottom) {
newY = currentPosition.y + (navbarBottom - elementTop)
}
}

currentPosition.x = newX
currentPosition.y = newY

// Apply the new position to the element using the CSS transform property
element.style.transform = `translate(${currentPosition.x}px, ${currentPosition.y}px)`
Expand Down
28 changes: 26 additions & 2 deletions v1/src/simulator/src/drag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,32 @@ function updatePosition(
// Update the element's x and y position
const currentPosition = positions.get(element)
if (!currentPosition) return // Check if the currentPosition is valid
currentPosition.x += dx
currentPosition.y += dy

// Calculate new position
let newX = currentPosition.x + dx
let newY = currentPosition.y + dy

// Get navbar bottom position to prevent overlap
const navbar = document.querySelector('.navbar.header') as HTMLElement
if (navbar) {
const navbarRect = navbar.getBoundingClientRect()
const navbarBottom = navbarRect.bottom

// Get element's current position on screen
const elementRect = element.getBoundingClientRect()
const elementTop = elementRect.top

// Calculate what the new top position would be after applying transform
const newElementTop = elementTop - currentPosition.y + newY

// Prevent panel from going above the navbar
if (newElementTop < navbarBottom) {
newY = currentPosition.y + (navbarBottom - elementTop)
}
}

currentPosition.x = newX
currentPosition.y = newY

// Apply the new position to the element using the CSS transform property
element.style.transform = `translate(${currentPosition.x}px, ${currentPosition.y}px)`
Expand Down