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

Fix an issue with not being able to group a shape an an arrow. #2205

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

MitjaBezensek
Copy link
Contributor

@MitjaBezensek MitjaBezensek commented Nov 13, 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

CleanShot.2023-11-13.at.16.19.47.mp4

Binding ungroups

CleanShot.2023-11-13.at.16.21.17.mp4

After

Can create the group

CleanShot.2023-11-13.at.16.20.14.mp4

Does not ungroup

CleanShot.2023-11-13.at.16.21.35.mp4

Fixes #2088
Fixes #2089

Change Type

  • patch — Bug fix
  • minor — New feature
  • major — Breaking change
  • dependencies — Changes to package dependencies1
  • documentation — Changes to the documentation only2
  • tests — Changes to any test code only2
  • internal — Any other changes that don't affect the published package2
  • I don't know

Test Plan

Testing that you can correctly group a shape and an arrow bound to it

  1. Insert a shape
  2. Insert an arrow and bind it to the shape
  3. Select both and group them (use the keyboard shortcut, seems like we disable the UI for this case).
  4. 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.

Footnotes

  1. publishes a patch release, for devDependencies use internal

  2. will not publish a new version 2 3

@huppy-bot huppy-bot bot added the bugfix Bug fix label Nov 13, 2023
Copy link

vercel bot commented Nov 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
examples ✅ Ready (Inspect) Visit Preview Nov 14, 2023 7:13am

Copy link
Collaborator

@ds300 ds300 left a comment

Choose a reason for hiding this comment

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

works for me 👍🏼

packages/editor/src/lib/editor/Editor.ts Outdated Show resolved Hide resolved
Co-authored-by: David Sheldrick <d.j.sheldrick@gmail.com>
@MitjaBezensek MitjaBezensek added this pull request to the merge queue Nov 14, 2023
Merged via the queue into main with commit 04e08b1 Nov 14, 2023
5 checks passed
@MitjaBezensek MitjaBezensek deleted the mitja/arrows-groups branch November 14, 2023 07:19
@MitjaBezensek MitjaBezensek added the needs qa Tag a pull request as needing to be manually QA'd before release label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fix needs qa Tag a pull request as needing to be manually QA'd before release
Projects
None yet
2 participants