-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
refactor(Group): parent
+ fix(ActiveSelection): transferring object
#9349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Build Stats
|
ShaMan123
commented
Sep 16, 2023
Comment on lines
+84
to
+90
// make sure we exit the parent only once | ||
if (object.parent && object.parent === object.group) { | ||
// keep the object part of the parent | ||
object.parent._exitGroup(object); | ||
} else if (object.group && object.parent !== object.group) { | ||
// in case `object` is transferred between active selections we remove it from the prev | ||
object.group.remove(object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is this fix to active selection
I need to assess if this should be merged into #8951 or done on top of it but they are duplicates to some extent |
ShaMan123
added a commit
that referenced
this pull request
Nov 7, 2023
commit 1395602 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Oct 18 18:23:35 2023 +0700 prettier commit d7fe84b Merge: d1f9b4d 7a71097 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Oct 18 18:19:27 2023 +0700 Merge branch 'master' into parent commit d1f9b4d Merge: 0b43b72 ef27fcd Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 23 14:00:34 2023 +0530 Merge branch 'master' into parent commit 0b43b72 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 23 13:33:25 2023 +0530 another parent/group fix commit 89e4c74 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 16 08:56:50 2023 +0530 e2e test Update index.spec.ts commit c2fc529 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 16 07:51:06 2023 +0530 comments commit a09f840 Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Date: Sat Sep 16 02:18:07 2023 +0000 update CHANGELOG.md commit b3bc476 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 16 07:37:01 2023 +0530 refactor(): `parent` + fix active selection edge case
closing => sqaushed into #8951 |
ShaMan123
added a commit
that referenced
this pull request
Nov 7, 2023
commit 1395602 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Oct 18 18:23:35 2023 +0700 prettier commit d7fe84b Merge: d1f9b4d 7a71097 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Oct 18 18:19:27 2023 +0700 Merge branch 'master' into parent commit d1f9b4d Merge: 0b43b72 ef27fcd Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 23 14:00:34 2023 +0530 Merge branch 'master' into parent commit 0b43b72 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 23 13:33:25 2023 +0530 another parent/group fix commit 89e4c74 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 16 08:56:50 2023 +0530 e2e test Update index.spec.ts commit c2fc529 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 16 07:51:06 2023 +0530 comments commit a09f840 Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Date: Sat Sep 16 02:18:07 2023 +0000 update CHANGELOG.md commit b3bc476 Author: ShaMan123 <shacharnen@gmail.com> Date: Sat Sep 16 07:37:01 2023 +0530 refactor(): `parent` + fix active selection edge case
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
I was looking into the patch I made into #9336 and understood that
getParent
was a design mistake.It is confusing and silly. I wrote it in a haste, not wanting to create too many changes.
It is a design mistake IMO mainly because there is a confusion about active selection - should it be considered a parent or not.
Apps might need to access both the the current
group
of an object and both the owning group.Since
group
andcanvas
are used it seems bad that to get the owning parent you dogetParent(true)
.I regret it.
This PR makes it simpler both for fabric and for devs and removes ambiguity of the
group
property.Please accept it before v6 so it only breaks beta.
Description
This PR introduces the
parent
property onObject
and removesgetParent
.The
parent
property is the owning group of an object, meaning not the active selection.While testing the PR I realized I didn't take into consideration another part of transferring an object between active selections that I missed in #9336
The object should be removed from the active selection and not be reomved from its owning group.
related to #8951
Changes
BREAKING beta: rm
getParent
Gist
In Action