-
-
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(): Canvas dom handling delegation to utility class #8930
Conversation
Build Stats
|
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.
Suggest order of review:
src/canvas/ElementsManager/StaticCanvasElements.ts
src/canvas/ElementsManager/util.ts
src/canvas/StaticCanvas.ts
src/canvas/ElementsManager/CanvasElements.ts
src/canvas/SelectableCanvas.ts
- the rest
src/canvas/ElementsManager/util.ts
Outdated
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 file might belong under the util dir
extract target find logic
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.
I am happy with this.
I believe it is ready for reviewing.
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.
done
This is small enough to be discussed. |
I didn't read the code deeply, but apart naming changes i think the targetFind part is rushed since it could fit in other kinds of changes, like the selection code and is not worth doing now. The layout logic of group is exactly something i wanted to refactor in this style. |
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.
- reverted target find 7a069c0
- renamed to DOMManager
- exported
- added dep warning on
Canvas#containerClass
I need to figure out a couple of things then i can merge.
I ll look at it tomorrow or later today |
I moved all the utilities that are used only by the dom manager in the dom manager folder. |
Motivation
Mine/Our discomfort of what is going on with prototype chains
Description
This is a POC here for discussion, show casing the concept of flattening the prototype chain of canvas into delegates/services/managers (call them how'd you like).
Each of them should have a well defined scope.
The final purpose being not having a prototype chain at all. Not for canvas nor for objects. Or at least minimizing significantly. So that subclassing, customizing etc. is more straight forward.
It should also make the code more easy to understand because of scoping.
I chose to begin with canvas deliberately to prove it can be done and prove it has value.
Took a lot more time than I thought.
Part of the concept is to make these changes only internal and non breaking. Then when the time is right swap everything, break and bump a version.
Changes
No changes to exposed interfaces, only to internals - NOT BREAKING
FindTarget
and exposedrenderTarget
setDimensions
Gist
In Action