Skip to content
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.

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

You should be able to group an arrow with a shape that it binds to #2089

Closed
SomeHats opened this issue Oct 17, 2023 · 0 comments · Fixed by #2205
Closed

You should be able to group an arrow with a shape that it binds to #2089

SomeHats opened this issue Oct 17, 2023 · 0 comments · Fixed by #2205

Comments

@SomeHats
Copy link
Contributor

You can group an arrow with a shape if it isn't bound to it. But you can't do it if it's grouped.

Image

github-merge-queue bot pushed a commit that referenced this issue Nov 14, 2023
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 #2088
Fixes #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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant