Skip to content

Commit

Permalink
refactor: drop threshold from reordering dashboard widgets (#8157)
Browse files Browse the repository at this point in the history
  • Loading branch information
@tomivirkki
tomivirkki authored Nov 15, 2024
1 parent d8edaff commit ad7b6d8
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 90 deletions.
51 changes: 45 additions & 6 deletions packages/dashboard/src/widget-reorder-controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import { getElementItem, getItemsArrayOfItem, itemsEqual, WRAPPER_LOCAL_NAME } from './vaadin-dashboard-helpers.js';

const REORDER_EVENT_TIMEOUT = 200;
const REORDER_MOVE_THRESHOLD = 10;

/**
* A controller to widget reordering inside a dashboard.
Expand Down Expand Up @@ -64,6 +65,29 @@ export class WidgetReorderController {
return;
}

// The location against which we measure the drag distance
this.__startX ||= e.clientX;
this.__startY ||= e.clientY;

// Delta values for the current drag event
const currentDx = e.clientX - (this.__previousX || e.clientX);
const currentDy = e.clientY - (this.__previousY || e.clientY);
this.__previousX = e.clientX;
this.__previousY = e.clientY;

if (currentDx && this.__dx && Math.sign(currentDx) !== Math.sign(this.__dx)) {
// If the direction of the drag changes, reset the start position
this.__startX = e.clientX;
}
if (currentDy && this.__dy && Math.sign(currentDy) !== Math.sign(this.__dy)) {
// If the direction of the drag changes, reset the start position
this.__startY = e.clientY;
}

// The delta values for the drag event against the start position
this.__dx = e.clientX - this.__startX;
this.__dy = e.clientY - this.__startY;

e.preventDefault();
e.dataTransfer.dropEffect = 'move';

Expand All @@ -88,7 +112,15 @@ export class WidgetReorderController {
}

// Check if the dragged element is dragged enough over the target element
if (!this.__reordering && this.__isDraggedOver(draggedElement, targetElement, e.clientX, e.clientY)) {
if (
!this.__reordering &&
this.__isDraggedOver(draggedElement, targetElement, {
x: e.clientX,
y: e.clientY,
dx: this.__dx,
dy: this.__dy,
})
) {
// Prevent reordering multiple times in quick succession
this.__reordering = true;
setTimeout(() => {
Expand All @@ -109,6 +141,13 @@ export class WidgetReorderController {
return;
}

this.__previousX = null;
this.__previousY = null;
this.__startX = null;
this.__startY = null;
this.__dx = null;
this.__dy = null;

// If the originally dragged element is restored to the DOM (as a direct child of the host),
// to make sure "dragend" event gets dispatched, remove it to avoid duplicates
if (this.__draggedElement.parentElement === this.host) {
Expand Down Expand Up @@ -169,21 +208,21 @@ export class WidgetReorderController {
* of the drag event.
* @private
*/
__isDraggedOver(draggedElement, targetElement, x, y) {
__isDraggedOver(draggedElement, targetElement, { x, y, dx, dy }) {
const draggedPos = draggedElement.getBoundingClientRect();
const targetPos = targetElement.getBoundingClientRect();
if (draggedPos.top >= targetPos.bottom) {
// target is on a row above the dragged widget
return y < targetPos.top + targetPos.height / 2;
return y < targetPos.bottom && dy < -REORDER_MOVE_THRESHOLD;
} else if (draggedPos.bottom <= targetPos.top) {
// target is on a row below the dragged widget
return y > targetPos.top + targetPos.height / 2;
return y > targetPos.top && dy > REORDER_MOVE_THRESHOLD;
} else if (draggedPos.left >= targetPos.right) {
// target is on a column to the left of the dragged widget
return x < targetPos.left + targetPos.width / 2;
return x < targetPos.right && dx < -REORDER_MOVE_THRESHOLD;
} else if (draggedPos.right <= targetPos.left) {
// target is on a column to the right of the dragged widget
return x > targetPos.left + targetPos.width / 2;
return x > targetPos.left && dx > REORDER_MOVE_THRESHOLD;
}
}

Expand Down
87 changes: 15 additions & 72 deletions packages/dashboard/test/dashboard-widget-reordering.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,6 @@ describe('dashboard - widget reordering', () => {
]);
});

it('should not reorder if dragged barely over another widget (start -> end)', async () => {
fireDragStart(getElementFromCell(dashboard, 0, 0)!);
await nextFrame();

fireDragOver(getElementFromCell(dashboard, 0, 1)!, 'start');
await nextFrame();

// prettier-ignore
expectLayout(dashboard, [
[0, 1],
]);
});

it('should reorder the widgets while dragging (end -> start)', async () => {
fireDragStart(getElementFromCell(dashboard, 0, 1)!);
await nextFrame();
Expand All @@ -105,19 +92,6 @@ describe('dashboard - widget reordering', () => {
]);
});

it('should not reorder if dragged barely over another widget (end -> start)', async () => {
fireDragStart(getElementFromCell(dashboard, 0, 1)!);
await nextFrame();

fireDragOver(getElementFromCell(dashboard, 0, 0)!, 'end');
await nextFrame();

// prettier-ignore
expectLayout(dashboard, [
[0, 1],
]);
});

it('should reorder the widgets while dragging (top -> bottom)', async () => {
dashboard.style.width = `${columnWidth}px`;
await onceResized(dashboard);
Expand All @@ -141,29 +115,6 @@ describe('dashboard - widget reordering', () => {
]);
});

it('should not reorder if dragged barely over another widget (top -> bottom)', async () => {
dashboard.style.width = `${columnWidth}px`;
await onceResized(dashboard);

// prettier-ignore
expectLayout(dashboard, [
[0],
[1],
]);

fireDragStart(getElementFromCell(dashboard, 0, 0)!);
await nextFrame();

fireDragOver(getElementFromCell(dashboard, 1, 0)!, 'top');
await nextFrame();

// prettier-ignore
expectLayout(dashboard, [
[0],
[1],
]);
});

it('should reorder the widgets while dragging (bottom -> top)', async () => {
dashboard.style.width = `${columnWidth}px`;
await onceResized(dashboard);
Expand All @@ -187,29 +138,6 @@ describe('dashboard - widget reordering', () => {
]);
});

it('should not reorder if dragged barely over another widget (bottom -> top)', async () => {
dashboard.style.width = `${columnWidth}px`;
await onceResized(dashboard);

// prettier-ignore
expectLayout(dashboard, [
[0],
[1],
]);

fireDragStart(getElementFromCell(dashboard, 1, 0)!);
await nextFrame();

fireDragOver(getElementFromCell(dashboard, 0, 0)!, 'bottom');
await nextFrame();

// prettier-ignore
expectLayout(dashboard, [
[0],
[1],
]);
});

// The sendMouse helper does not seem to work well with Firefox
(isFirefox ? it.skip : it)('should reorder using native DnD', async () => {
const spy = sinon.spy();
Expand All @@ -221,6 +149,17 @@ describe('dashboard - widget reordering', () => {
await sendMouse({
type: 'down',
});
// Start moving the mouse
const draggedWidget = getElementFromCell(dashboard, 0, 0);
const draggedWidgetRect = draggedWidget!.getBoundingClientRect();
const startY = draggedWidgetRect.top + draggedWidgetRect.height / 2;
const startX = document.dir === 'rtl' ? draggedWidgetRect.right - 1 : draggedWidgetRect.left + 1;
// await sendMouse({ type: 'move', position: [Math.round(startX), Math.round(startY)] });
// Workaround an issue with sendMouse not supporting multiple subsequent moves
const reorderController = (dashboard as any).__widgetReorderController;
reorderController.__startX = startX;
reorderController.__startY = startY;

// Move the mouse to the second widget
const secondWidget = getElementFromCell(dashboard, 0, 1)!;
const secondWidgetRect = secondWidget.getBoundingClientRect();
Expand Down Expand Up @@ -471,6 +410,10 @@ describe('dashboard - widget reordering', () => {
[0],
]);

// Workaround test limitations to have proper delta on the next dragover event
fireDragOver(getElementFromCell(dashboard, 1, 0)!, 'bottom');
fireDragOver(getElementFromCell(dashboard, 1, 0)!, 'top');

// Drag over the new widget 2
resetReorderTimeout(dashboard);
fireDragOver(getElementFromCell(dashboard, 0, 1)!, 'top');
Expand Down
5 changes: 4 additions & 1 deletion packages/dashboard/test/dashboard.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ describe('dashboard', () => {
it('should render a new widget', async () => {
dashboard.items = [...dashboard.items, { id: '2' }];
await nextFrame();
await nextFrame();

const newWidget = getElementFromCell(dashboard, 1, 0);
expect(newWidget).to.be.ok;
Expand Down Expand Up @@ -117,12 +118,14 @@ describe('dashboard', () => {
expect(spy.firstCall.args[0].detail.items).to.eql([{ id: '0' }]);
});

it('should not dispatch an item-remove event', () => {
it('should not dispatch an item-remove event', async () => {
const spy = sinon.spy();
// @ts-ignore unexpected event type
dashboard.addEventListener('item-remove', spy);
await nextFrame();
const widget = getElementFromCell(dashboard, 0, 1);
getRemoveButton(widget as DashboardWidget).click();
await nextFrame();
expect(spy).to.not.be.called;
});

Expand Down
31 changes: 20 additions & 11 deletions packages/dashboard/test/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,17 +224,6 @@ export function createDragEvent(type: string, { x, y }: { x: number; y: number }
return event;
}

export function fireDragStart(dragStartTarget: Element): TestDragEvent {
const draggable = getDraggable(dragStartTarget);
const draggableRect = draggable.getBoundingClientRect();
const event = createDragEvent('dragstart', {
x: draggableRect.left + draggableRect.width / 2,
y: draggableRect.top + draggableRect.height / 2,
});
draggable.dispatchEvent(event);
return event;
}

function getEventCoordinates(relativeElement: Element, location: 'top' | 'bottom' | 'start' | 'end') {
const { top, bottom, left, right } = relativeElement.getBoundingClientRect();
const y = location === 'top' ? top : bottom;
Expand All @@ -249,6 +238,26 @@ export function fireDragOver(dragOverTarget: Element, location: 'top' | 'bottom'
return event;
}

export function fireDragStart(dragStartTarget: Element): TestDragEvent {
const draggable = getDraggable(dragStartTarget);
const draggableRect = draggable.getBoundingClientRect();
const event = createDragEvent('dragstart', {
x: draggableRect.left + draggableRect.width / 2,
y: draggableRect.top + draggableRect.height / 2,
});
draggable.dispatchEvent(event);

// Dispatch initial dragover event to make sure any following dragover has some delta
draggable.dispatchEvent(
createDragEvent('dragover', {
x: draggableRect.left + draggableRect.width / 2,
y: draggableRect.top + draggableRect.height / 2,
}),
);

return event;
}

export function fireDragEnd(dashboard: Element): TestDragEvent {
const event = createDragEvent('dragend', { x: 0, y: 0 });
dashboard.dispatchEvent(event);
Expand Down

0 comments on commit ad7b6d8

Please sign in to comment.