Fix an issue with not being able to group a shape an an arrow. (#2205)
There was an issue with preventing grouping of a shape and an arrow bound to it. There was another issue where you had a shape and an unbound arrow grouped. If you then tried to bind the arrow to the shape it would ungroup the two. The underlying issue for both was the same and it goes something like this: 1. We group the shape and the bound arrow. 2. This reparents both of them to the group. 3. This triggers `registerAfterChangeHandler` cb. 4. This reparents the arrow and it reparents it to the page since we only have one binding. 5. This then triggers `onChildrenChange` in `GroupShapeUtil` which removes the group. ## Before **Cant create the group** https://github.com/tldraw/tldraw/assets/2523721/d6717b8a-9a68-484b-bf2d-969140a9bfc1 **Binding ungroups** https://github.com/tldraw/tldraw/assets/2523721/c85f99d5-9343-454f-a934-85d7489dbc72 ## After **Can create the group** https://github.com/tldraw/tldraw/assets/2523721/e6c026d8-6a42-413c-9471-30669610910b **Does not ungroup** https://github.com/tldraw/tldraw/assets/2523721/74e43741-31a9-42a2-b1e0-6dca2e678669 Fixes https://github.com/tldraw/tldraw/issues/2088 Fixes https://github.com/tldraw/tldraw/issues/2089 ### 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 **Testing that you can correctly group a shape and an arrow bound to it** 1. Insert a shape 7. Insert an arrow and bind it to the shape 8. Select both and group them (use the keyboard shortcut, seems like we disable the UI for this case). 9. This should create a group. **Testing that you don't ungroup an arrow when you unbind it from a shape** 1. Start with a group that contains a shape and an arrow. 2. Bind the arrow to the shape and then unbind it. 3. The group should still be there. - [ ] Unit Tests - [ ] End to end tests ### Release Notes - Add a brief release note for your PR here. --------- Co-authored-by: David Sheldrick <d.j.sheldrick@gmail.com>
This commit is contained in:
parent
daf729d45c
commit
04e08b1871
2 changed files with 39 additions and 3 deletions
|
@ -290,8 +290,14 @@ export class Editor extends EventEmitter<TLEventMap> {
|
||||||
// if arrow has two bindings, always parent arrow to closest common ancestor of the bindings
|
// if arrow has two bindings, always parent arrow to closest common ancestor of the bindings
|
||||||
nextParentId = this.findCommonAncestor([startShape, endShape]) ?? parentPageId
|
nextParentId = this.findCommonAncestor([startShape, endShape]) ?? parentPageId
|
||||||
} else if (startShape || endShape) {
|
} else if (startShape || endShape) {
|
||||||
|
const bindingParentId = (startShape || endShape)?.parentId
|
||||||
|
// If the arrow and the shape that it is bound to have the same parent, then keep that parent
|
||||||
|
if (bindingParentId && bindingParentId === arrow.parentId) {
|
||||||
|
nextParentId = arrow.parentId
|
||||||
|
} else {
|
||||||
// if arrow has one binding, keep arrow on its own page
|
// if arrow has one binding, keep arrow on its own page
|
||||||
nextParentId = parentPageId
|
nextParentId = parentPageId
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
|
@ -1,6 +1,8 @@
|
||||||
import {
|
import {
|
||||||
Editor,
|
Editor,
|
||||||
TLArrowShape,
|
TLArrowShape,
|
||||||
|
TLShape,
|
||||||
|
TLShapeId,
|
||||||
assert,
|
assert,
|
||||||
exhaustiveSwitchError,
|
exhaustiveSwitchError,
|
||||||
useEditor,
|
useEditor,
|
||||||
|
@ -163,10 +165,38 @@ export const useThreeStackableItems = () => {
|
||||||
return useValue('threeStackableItems', () => shapesWithUnboundArrows(editor).length > 2, [editor])
|
return useValue('threeStackableItems', () => shapesWithUnboundArrows(editor).length > 2, [editor])
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function shapesWithArrowsBoundToThem(editor: Editor) {
|
||||||
|
const selectedShapes = editor.getSelectedShapes()
|
||||||
|
const arrows: TLArrowShape[] = []
|
||||||
|
const otherShapesMap = new Map<TLShapeId, TLShape>()
|
||||||
|
selectedShapes.forEach((shape) => {
|
||||||
|
if (shape.type === 'arrow') {
|
||||||
|
arrows.push(shape as TLArrowShape)
|
||||||
|
} else {
|
||||||
|
otherShapesMap.set(shape.id, shape)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
// We want to get all the arrows that are either unbound or bound to one of the selected shapes
|
||||||
|
const groupableArrows = arrows.filter((arrow) => {
|
||||||
|
if (arrow.props.start.type === 'binding') {
|
||||||
|
if (!otherShapesMap.has(arrow.props.start.boundShapeId)) {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if (arrow.props.end.type === 'binding') {
|
||||||
|
if (!otherShapesMap.has(arrow.props.end.boundShapeId)) {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return true
|
||||||
|
})
|
||||||
|
return Array.from(otherShapesMap.values()).concat(groupableArrows)
|
||||||
|
}
|
||||||
|
|
||||||
/** @internal */
|
/** @internal */
|
||||||
export const useAllowGroup = () => {
|
export const useAllowGroup = () => {
|
||||||
const editor = useEditor()
|
const editor = useEditor()
|
||||||
return useValue('allowGroup', () => shapesWithUnboundArrows(editor).length > 1, [editor])
|
return useValue('allowGroup', () => shapesWithArrowsBoundToThem(editor).length > 1, [editor])
|
||||||
}
|
}
|
||||||
|
|
||||||
/** @internal */
|
/** @internal */
|
||||||
|
|
Loading…
Reference in a new issue