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

feat(Group): First Patch of New Group! 🥳 #7858

Merged
merged 4 commits into from
Apr 7, 2022
Merged

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Apr 3, 2022

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 and stateProperties. 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:

  • eventually specify the arguments in the functions.
  • consider wrapping useless calls ( with no objects for example ) wit ha DEV_START regex and a console warning
  • remove has a didRemove logic, while add and insertAt don't

misc.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

  • removed the specific tests for add/remove/insert add and add generic one. ( tests were duplicated ? )

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

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.07 |     75.8 |   86.33 |   82.78 |                                               
 fabric.js |   83.07 |     75.8 |   86.33 |   82.78 | ...,30291,30416,30496-30561,30684,30783,31019 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 changed the title feat(Group): First Group Patch! 🥳 feat(Group): First New Group Patch! 🥳 Apr 3, 2022
@ShaMan123 ShaMan123 requested a review from asturur April 3, 2022 20:49
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.12 |    75.84 |   86.33 |   82.83 |                                               
 fabric.js |   83.12 |    75.84 |   86.33 |   82.83 | ...,30291,30416,30496-30561,30684,30783,31019 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 changed the title feat(Group): First New Group Patch! 🥳 feat(Group): First Patch of New Group! 🥳 Apr 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.98 |    75.69 |   86.26 |   82.69 |                                               
 fabric.js |   82.98 |    75.69 |   86.26 |   82.69 | ...,30291,30416,30496-30561,30684,30783,31019 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123
Copy link
Contributor Author

@asturur can we merge this so I can finalize the rest of the PRs?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.89 |    75.33 |   86.26 |    82.6 |                                               
 fabric.js |   82.89 |    75.33 |   86.26 |    82.6 | ...,30453,30527,30538-30603,30726,30825,31061 
-----------|---------|----------|---------|---------|-----------------------------------------------

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment