-
-
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
patch(TS): canvas patch init 2 #8520
Conversation
This reverts commit 286967c.
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.
READY
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.
updated from master and rebased
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.
ready
this.cacheCanvasEl.setAttribute('width', `${this.width}`); | ||
this.cacheCanvasEl.setAttribute('height', `${this.height}`); |
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.
size will be set by setDimensions
on init
/** | ||
* @private | ||
*/ | ||
private _bindEvents() { |
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.
no need to have this now that it is impossible to init canvas twice
src/canvas/static_canvas.class.ts
Outdated
renderAndResetBound: () => void; | ||
requestRenderAllBound: () => void; |
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.
dropped
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.
mergable
src/canvas/static_canvas.class.ts
Outdated
this.renderAndReset = this.renderAndReset.bind(this); | ||
this.requestRenderAll = this.requestRenderAll.bind(this); |
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.
these were renamed,
see description
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.
Yes yes i have read everything i was in the middle of things and then i had to go to bed.
Probably going to let them both names.
Bind on constructor is a great normal thing we should have done as soon as we started moving canvas, i really didn't thin of it.
The reorganization is great, i don't like the idea of adding a parameter to setDimensions to have it skip rendering just because we need it for initialization, since we managed to remove eventsBound: boolean, let's not add a similar one. |
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 absolutely agree
it is how it was before and i wouldn't mind if it doesn't save half kb on the minified build. |
Motivation
Description
Refactor canvas init logic, more work can be done in this scope
Changes
requestRenderAllBound
,renderAndResetBound
fix_initRetinaScaling
regression from chore(TS): Convert Canvas to TS #8510, BREAKING since now should be called only ifisRetinaScaling
(doesn't check in the method as previously did)Gist
In Action