-
-
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
chore(TS): Collection #8433
chore(TS): Collection #8433
Conversation
Build Stats
|
Fabric.Collection wasn't really a public interface, whoever was extending from that for custom use cases can still use the createCollectionMixin. Changes are good, i want review tomorrow morning slowly and merge this |
class extends createCollectionMixin(CommonMethods) { | ||
add(...objects: FabricObject[]) { | ||
super.add(...objects); | ||
objects.length > 0 && this.renderOnAddRemove && this.requestRenderAll(); |
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.
One day i would like to go over all of those this.renderOnAddRemove
and decide if we really need them.
Those are good for demos because you have to write less, but sometimes boring during normal application development becuase sometimes you are doing something complex and you never want to render automatically.
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.
+1
Perhaps this is part of the scope of #8372
Fabulous! |
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
Motivation
Description
Changes
insertAt
signature -(objects, index)
=>(index, ...objects)
. Done to comply with the rest of the methods and resemblance toArray.splice
fabric.Collection
, but I can put it backGist
Moved all methods using
super
call to a temporary super class of Group/Canvas.Once both are migrated the super class won't be necessary
In Action