From f510dc2452dd07553c70debe0de6c9fe237fec78 Mon Sep 17 00:00:00 2001 From: Takuto Mori Gump Date: Tue, 19 Sep 2023 21:14:04 +0900 Subject: [PATCH] [fix] Moving group items inside of a frame (dropping) (#1886) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes: #1884 Fix bug: ungroup when moving a shape in a group in a frame. ### before https://github.com/tldraw/tldraw/assets/24749358/994a871a-5da2-4d8b-b80e-a4db43e66142 ### after https://github.com/tldraw/tldraw/assets/24749358/4395d416-b7f6-4af0-a4c2-0bf77c29d7e2 ### Change Type - [x] `patch` — Bug fix - [ ] `minor` — New feature - [ ] `major` — Breaking change - [ ] `dependencies` — Changes to package dependencies[^1] - [ ] `documentation` — Changes to the documentation only[^2] - [ ] `tests` — Changes to any test code only[^2] - [ ] `internal` — Any other changes that don't affect the published package[^2] - [ ] I don't know [^1]: publishes a `patch` release, for devDependencies use `internal` [^2]: will not publish a new version ### Test Plan 1. Add a step-by-step description of how to test your PR here. 2. - [ ] Unit Tests - [ ] End to end tests ### Release Notes - Fix bug: ungroup when moving a shape in a group in a frame. --------- Co-authored-by: Steve Ruiz Co-authored-by: David Sheldrick --- packages/editor/src/lib/editor/Editor.ts | 1 + .../src/lib/shapes/frame/FrameShapeUtil.tsx | 11 +-- .../tools/SelectTool/DragAndDropManager.ts | 54 +++++++-------- .../tools/SelectTool/children/Translating.ts | 1 + packages/tldraw/src/test/frames.test.ts | 67 ++++++++++++++++++- 5 files changed, 97 insertions(+), 37 deletions(-) diff --git a/packages/editor/src/lib/editor/Editor.ts b/packages/editor/src/lib/editor/Editor.ts index dd3b4548b..c2ed81c9e 100644 --- a/packages/editor/src/lib/editor/Editor.ts +++ b/packages/editor/src/lib/editor/Editor.ts @@ -4900,6 +4900,7 @@ export class Editor extends EventEmitter { // Only allow dropping into the masked page bounds of the shape, e.g. when a frame is // partially clipped by its own parent frame const maskedPageBounds = this.getShapeMaskedPageBounds(shape.id) + if ( maskedPageBounds && maskedPageBounds.containsPoint(point) && diff --git a/packages/tldraw/src/lib/shapes/frame/FrameShapeUtil.tsx b/packages/tldraw/src/lib/shapes/frame/FrameShapeUtil.tsx index 51c93ac73..e5b4671b9 100644 --- a/packages/tldraw/src/lib/shapes/frame/FrameShapeUtil.tsx +++ b/packages/tldraw/src/lib/shapes/frame/FrameShapeUtil.tsx @@ -205,16 +205,11 @@ export class FrameShapeUtil extends BaseBoxShapeUtil { // If frame is in a group, keep the shape // moved out in that group + if (isInGroup) { - this.editor.reparentShapes( - shapes.map((shape) => shape.id), - parent.id - ) + this.editor.reparentShapes(shapes, parent.id) } else { - this.editor.reparentShapes( - shapes.map((shape) => shape.id), - this.editor.currentPageId - ) + this.editor.reparentShapes(shapes, this.editor.currentPageId) } } diff --git a/packages/tldraw/src/lib/tools/SelectTool/DragAndDropManager.ts b/packages/tldraw/src/lib/tools/SelectTool/DragAndDropManager.ts index f273a01c1..4ce2681b9 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/DragAndDropManager.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/DragAndDropManager.ts @@ -1,4 +1,4 @@ -import { Editor, TLShape, TLShapeId, compact } from '@tldraw/editor' +import { Editor, TLShape, TLShapeId, Vec2d, compact } from '@tldraw/editor' const LAG_DURATION = 100 @@ -9,15 +9,20 @@ export class DragAndDropManager { } prevDroppingShapeId: TLShapeId | null = null - currDroppingShapeId: TLShapeId | null = null droppingNodeTimer: ReturnType | null = null + first = true + updateDroppingNode(movingShapes: TLShape[], cb: () => void) { + if (this.first) { + this.prevDroppingShapeId = + this.editor.getDroppingOverShape(this.editor.inputs.originPagePoint, movingShapes)?.id ?? + null + this.first = false + } + if (this.droppingNodeTimer === null) { - const { currentPagePoint } = this.editor.inputs - this.currDroppingShapeId = - this.editor.getDroppingOverShape(currentPagePoint, movingShapes)?.id ?? null this.setDragTimer(movingShapes, LAG_DURATION * 10, cb) } else if (this.editor.inputs.pointerVelocity.len() > 0.5) { clearInterval(this.droppingNodeTimer) @@ -28,39 +33,31 @@ export class DragAndDropManager { private setDragTimer(movingShapes: TLShape[], duration: number, cb: () => void) { this.droppingNodeTimer = setTimeout(() => { this.editor.batch(() => { - this.handleDrag(movingShapes, cb) + this.handleDrag(this.editor.inputs.currentPagePoint, movingShapes, cb) }) this.droppingNodeTimer = null }, duration) } - private handleDrag(movingShapes: TLShape[], cb?: () => void) { - const { currentPagePoint } = this.editor.inputs - + private handleDrag(point: Vec2d, movingShapes: TLShape[], cb?: () => void) { movingShapes = compact(movingShapes.map((shape) => this.editor.getShape(shape.id))) - const currDroppingShapeId = - this.editor.getDroppingOverShape(currentPagePoint, movingShapes)?.id ?? null + const nextDroppingShapeId = this.editor.getDroppingOverShape(point, movingShapes)?.id ?? null - if (currDroppingShapeId !== this.currDroppingShapeId) { - this.prevDroppingShapeId = this.currDroppingShapeId - this.currDroppingShapeId = currDroppingShapeId - } - - const { prevDroppingShapeId } = this - - if (currDroppingShapeId === prevDroppingShapeId) { - // we already called onDragShapesOver on this node, no need to do it again + // is the next dropping shape id different than the last one? + if (nextDroppingShapeId === this.prevDroppingShapeId) { return } + // the old previous one + const { prevDroppingShapeId } = this + const prevDroppingShape = prevDroppingShapeId && this.editor.getShape(prevDroppingShapeId) - const nextDroppingShape = currDroppingShapeId && this.editor.getShape(currDroppingShapeId) + const nextDroppingShape = nextDroppingShapeId && this.editor.getShape(nextDroppingShapeId) // Even if we don't have a next dropping shape id (i.e. if we're dropping // onto the page) set the prev to the current, to avoid repeat calls to // the previous parent's onDragShapesOut - this.prevDroppingShapeId = this.currDroppingShapeId if (prevDroppingShape) { this.editor.getShapeUtil(prevDroppingShape).onDragShapesOut?.(prevDroppingShape, movingShapes) @@ -80,15 +77,18 @@ export class DragAndDropManager { } cb?.() + + // next -> curr + this.prevDroppingShapeId = nextDroppingShapeId } dropShapes(shapes: TLShape[]) { - const { currDroppingShapeId } = this + const { prevDroppingShapeId } = this - this.handleDrag(shapes) + this.handleDrag(this.editor.inputs.currentPagePoint, shapes) - if (currDroppingShapeId) { - const shape = this.editor.getShape(currDroppingShapeId) + if (prevDroppingShapeId) { + const shape = this.editor.getShape(prevDroppingShapeId) if (!shape) return this.editor.getShapeUtil(shape).onDropShapesOver?.(shape, shapes) } @@ -96,7 +96,6 @@ export class DragAndDropManager { clear() { this.prevDroppingShapeId = null - this.currDroppingShapeId = null if (this.droppingNodeTimer !== null) { clearInterval(this.droppingNodeTimer) @@ -104,6 +103,7 @@ export class DragAndDropManager { this.droppingNodeTimer = null this.editor.setHintingShapes([]) + this.first = true } dispose = () => { diff --git a/packages/tldraw/src/lib/tools/SelectTool/children/Translating.ts b/packages/tldraw/src/lib/tools/SelectTool/children/Translating.ts index 6a592d639..f975bd0e2 100644 --- a/packages/tldraw/src/lib/tools/SelectTool/children/Translating.ts +++ b/packages/tldraw/src/lib/tools/SelectTool/children/Translating.ts @@ -202,6 +202,7 @@ export class Translating extends StateNode { if (changes.length > 0) { this.editor.updateShapes(changes) } + this.editor.setHoveredShape(null) } diff --git a/packages/tldraw/src/test/frames.test.ts b/packages/tldraw/src/test/frames.test.ts index e29219a25..91dbf6494 100644 --- a/packages/tldraw/src/test/frames.test.ts +++ b/packages/tldraw/src/test/frames.test.ts @@ -248,9 +248,9 @@ describe('frame shapes', () => { expect(editor.onlySelectedShape!.parentId).toBe(editor.currentPageId) editor.setCurrentTool('select') - editor.pointerDown(275, 275, ids.boxA).pointerMove(150, 150) + editor.pointerDown(275, 275).pointerMove(150, 150) - jest.advanceTimersByTime(250) + jest.advanceTimersByTime(300) expect(editor.onlySelectedShape!.id).toBe(ids.boxA) expect(editor.onlySelectedShape!.parentId).toBe(frameId) @@ -712,3 +712,66 @@ test('arrows bound to a shape within a group within a frame are reparented if th // expect arrow index to be greater than group index expect(editor.getShape(arrowId)?.index.localeCompare(editor.getShape(groupId)!.index)).toBe(1) }) + +describe('When dragging a shape inside a group inside a frame', () => { + const ids = { + frame1: createShapeId('frame'), + box1: createShapeId('geo1'), + box2: createShapeId('geo2'), + group1: createShapeId('group1'), + } + + beforeEach(() => { + editor.createShapes([ + { id: ids.frame1, type: 'frame', x: 0, y: 0, props: { w: 500, h: 500 } }, + { id: ids.box1, type: 'geo', parentId: ids.frame1, x: 100, y: 100 }, + { id: ids.box2, type: 'geo', parentId: ids.frame1, x: 300, y: 300 }, + ]) + }) + + it('When dragging a shape out of a frame', () => { + editor.select(ids.box1, ids.box2) + + expect(editor.selectedShapeIds).toHaveLength(2) + + editor.groupShapes(editor.selectedShapeIds, ids.group1) + + expect(editor.getShape(ids.box1)!.parentId).toBe(ids.group1) + + editor.pointerMove(100, 100).click().click() + + expect(editor.onlySelectedShape?.id).toBe(ids.box1) + + editor.pointerMove(150, 150).pointerDown().pointerMove(140, 140) + + jest.advanceTimersByTime(300) + + expect(editor.getShape(ids.box1)!.parentId).toBe(ids.group1) + }) + + it('reparents the shape to the page if it leaves the frame', () => { + editor.select(ids.box1, ids.box2) + + expect(editor.selectedShapeIds).toHaveLength(2) + + editor.groupShapes(editor.selectedShapeIds, ids.group1) + + expect(editor.getShape(ids.box1)!.parentId).toBe(ids.group1) + + editor.pointerMove(100, 100).click().click() + + expect(editor.onlySelectedShape?.id).toBe(ids.box1) + expect(editor.focusedGroupId).toBe(ids.group1) + + editor + .pointerMove(150, 150) + .pointerDown() + .pointerMove(-200, -200) + .pointerMove(-200, -200) + .pointerUp() + + jest.advanceTimersByTime(300) + + expect(editor.getShape(ids.box1)!.parentId).toBe(editor.currentPageId) + }) +})