-
-
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
feat(Group): First Patch of New Group! 🥳 #7858
Merged
Merged
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
Code Coverage Summary
|
ShaMan123
changed the title
feat(Group): First Group Patch! 🥳
feat(Group): First New Group Patch! 🥳
Apr 3, 2022
Code Coverage Summary
|
ShaMan123
changed the title
feat(Group): First New Group Patch! 🥳
feat(Group): First Patch of New Group! 🥳
Apr 3, 2022
Code Coverage Summary
|
This was referenced Apr 3, 2022
Closed
@asturur can we merge this so I can finalize the rest of the PRs? |
Code Coverage Summary
|
asturur
approved these changes
Apr 7, 2022
This was
linked to
issues
Apr 10, 2022
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.
There is no new feature that can be seen, but the group class now is able to self adjust the bounding box.
closes #6499, closes #3012, closes #7130, might close #7142, closes #7598
CHANGES compared to master
canvas_grouping.mixin.js
removed addWithUpdate and removeWithUpdate with add and remove.
collection.mixin.js
add insertAt and remove now take an array of items rather than a single item and they take a callback.
the callback replace
_onObjectAdded
and_onObjectRemoved
functions in the classes.The reason for this is that a classes may want to do different things when adding or removing for different reasons.
Add Remove and InsertAt are now the
withUpdate
version, and the without update does not exist anymore.getObjects now can take more types.
Worth to note that as of now the collection add/remove are never used directly but always wrapped in another method from
the class that inherits from collection.
active_selection.class.js
Not much changed.
toGroup became exitGroup ( verify if the name of the method sound correct, together with enterGroup in group )
object.class.js
The change isn't related to the PR and sneak in for some reason, probably because in looking at UTs changes i noticed a strong dependency between
includeDefaultValues
andstateProperties
. Depending how many UT fails removing that change it could be worth putting it in its own commit.static_canvas.class.js
add the internal implementation for add, remove and insertAt followed by a possible renderAll.
The implementation just call the collection one.
Notes:
didRemove
logic, while add and insertAt don'tmisc.js
Removed the dependency of SVG width/height in the groupSVGElements.
Before those changes group allowed to specify which was the centerPoint for the group and run special logic on it. Was an hack from when removed the PathGroup that was a specific container for parsed SVGs.
groupSVGElements can simply group thing. The specific logic that is there about trying to keep the SVG viewport was an hack and should be implemented with a clipPath or some other render logic, or could be ignored and developers need to clean the SVG from cross border objects.
group.class.js
Complete rewrite of the class.
Added a layout property to define the behavour of the group. More layout type will be defined in following PRs or supported as subclasses, depending on what seems to make more sense.
Added interactive property, for now disabled, used to permit to interact with objects inside the group
The initialization now attach an event listener to objects for modified, changed and selected/deselected.
When those event fire, the group will trigger a layout change.
(more to come)
change to tests
Activeselection
Canvas
Nothing ( swapped removeWIthUpdate and AddWithUpdate with remove/add and added a couple of setCoords after a change in subTargetCheck, that is completely fine. No real functional changes, but the setCoords required needs to be noted in the changelog )
StaticCanvas
extended the test about renderOnAddRemove, nothing else changed.
Collection
Adapted tests to the new add/remove/insert att argument signature with a callback.
Removed the constraints about chainability
Group
removed chainability constrains for add, remove, insertAt
Change the object shape to consider the new properties
rename destroy with removeAll