-
-
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): Convert object geometry in its own class #8390
Conversation
i think i m at 50% of this file and till now i didn't find any issue or bad thing |
export class Group extends FabricObject { | ||
|
||
} | ||
|
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.
just to make typings happy for now
Build Stats
*inaccurate, see link |
Ok i have 29 Unit test to address by the code is done, apart the part that needs to be changed to get back the UTs in shape. |
22 UTs to go |
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.
- fixed typos
- fixed buildStats diff sign
- removed duplicate TBoundingBox => TBBox
- added comment regarding
getScaledWidth/Height
I think we should deprecate them and exposegetSize
that will return the size in canvas plane
* Returns the coordinates of the object based on center coordinates | ||
* @param {Point} point The point which corresponds to the originX and originY params | ||
* @return {Point} | ||
*/ | ||
// getOriginPoint: function(center) { | ||
// return this.translateToOriginPoint(center, this.originX, this.originY); | ||
// }, |
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.
deleted forever
adjustPosition: function (to) { | ||
var angle = degreesToRadians(this.angle), | ||
hypotFull = this.getScaledWidth(), | ||
xFull = fabric.util.cos(angle) * hypotFull, | ||
yFull = fabric.util.sin(angle) * hypotFull, | ||
offsetFrom, | ||
offsetTo; | ||
|
||
//TODO: this function does not consider mixed situation like top, center. | ||
if (typeof this.originX === 'string') { | ||
offsetFrom = originXOffset[this.originX]; | ||
} else { | ||
offsetFrom = this.originX - 0.5; | ||
} | ||
if (typeof to === 'string') { | ||
offsetTo = originXOffset[to]; | ||
} else { | ||
offsetTo = to - 0.5; | ||
} | ||
this.left += xFull * (offsetTo - offsetFrom); | ||
this.top += yFull * (offsetTo - offsetFrom); | ||
this.setCoords(); | ||
this.originX = to; | ||
}, |
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.
deleted forever
* Sets the origin/position of the object to it's center point | ||
* @private | ||
* @return {void} | ||
*/ | ||
_setOriginToCenter() { | ||
this._originalOriginX = this.originX; | ||
this._originalOriginY = this.originY; | ||
|
||
const center = this.getRelativeCenterPoint(); | ||
|
||
this.originX = 'center'; | ||
this.originY = 'center'; | ||
|
||
this.left = center.x; | ||
this.top = center.y; | ||
} | ||
|
||
/** | ||
* Resets the origin/position of the object to it's original origin | ||
* @private | ||
* @return {void} | ||
*/ | ||
_resetOrigin() { | ||
if ( | ||
this._originalOriginX !== undefined && | ||
this._originalOriginY !== undefined | ||
) { | ||
const originPoint = this.translateToOriginPoint( | ||
this.getRelativeCenterPoint(), | ||
this._originalOriginX, | ||
this._originalOriginY | ||
); | ||
|
||
this.left = originPoint.x; | ||
this.top = originPoint.y; | ||
|
||
this.originX = this._originalOriginX; | ||
this.originY = this._originalOriginY; | ||
this._originalOriginX = undefined; | ||
this._originalOriginY = undefined; | ||
} |
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.
would love to refactor those to be just inside the object.rotate method, and remove those as standalone methods. Not today
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.
Agreed
I would add normalizePoint
to the list.
I think it is used only by controls.
@ShaMan123 ok this is it. |
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.
Looks great
From all I commented I would rename _getTransformedDimensions
ASAP
I guess you would prefer a dedicated PR.
Great job!
* @param {Boolean} calculate use coordinates of current position instead of .oCoords | ||
* @return {Boolean} true if the object contains the point | ||
*/ | ||
_containsCenterOfCanvas( |
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.
should be private:
Does not make sense outside the context of isOnScreen and isPartiallyOnScreen
flipX: this.flipX, | ||
flipY: this.flipY, | ||
}, | ||
value = composeMatrix(options); |
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.
Here is where I think of trying the skewY trick
* @type Number | ||
* @default 1 | ||
*/ | ||
strokeWidth: number; |
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 know this class requires strokeWidth and strokeUniform but I think they belong in an abstract super class
Thoughts?
But we can move ahead and think about this in a follow up
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.
maybe yes i don't want to over engineer, no one is going to look at those classes and they are going to look at Object only or Object + interaction.
I want to put a target on eoy for finishing this thing and all the changes that do not touch the public api can be postponed for me.
* @private | ||
* @returns {Point} dimensions | ||
*/ | ||
_getTransformedDimensions(options: any = {}): Point { |
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 want to rename this important method to getDimensions
or calcDimensions
I think it should deprecate getScaledWidth/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.
let's have a renaming task for all of those, but let's talk about it in a video call so that we can actually have a plan, and then we just go and rename. Renaming needs to be done before release of course.
skewX: this.skewX, | ||
skewY: this.skewY, | ||
width: this.width, | ||
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.
I put width and height here a while back
But now (after thinking of a fix to skewing controls + stroke projection) I understand it is wrong.
I makes no sense that a consumer of this method will provide the size.
Does it? The whole point of the method is to return the size of transform.
Passing width/height lets you "project" the bbox with a transform....
Thoughts?
offsetY = resolveOrigin(toOriginY) - resolveOrigin(fromOriginY); | ||
|
||
if (offsetX || offsetY) { | ||
const dim = this._getTransformedDimensions(); |
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 part I didn't touch in the group PRs but it requires thought
This method is restricted to the containing plane of the object.
Should it be? Is it confusing?
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.
Of course changing this method means chaning all the rest.
* Sets the origin/position of the object to it's center point | ||
* @private | ||
* @return {void} | ||
*/ | ||
_setOriginToCenter() { | ||
this._originalOriginX = this.originX; | ||
this._originalOriginY = this.originY; | ||
|
||
const center = this.getRelativeCenterPoint(); | ||
|
||
this.originX = 'center'; | ||
this.originY = 'center'; | ||
|
||
this.left = center.x; | ||
this.top = center.y; | ||
} | ||
|
||
/** | ||
* Resets the origin/position of the object to it's original origin | ||
* @private | ||
* @return {void} | ||
*/ | ||
_resetOrigin() { | ||
if ( | ||
this._originalOriginX !== undefined && | ||
this._originalOriginY !== undefined | ||
) { | ||
const originPoint = this.translateToOriginPoint( | ||
this.getRelativeCenterPoint(), | ||
this._originalOriginX, | ||
this._originalOriginY | ||
); | ||
|
||
this.left = originPoint.x; | ||
this.top = originPoint.y; | ||
|
||
this.originX = this._originalOriginX; | ||
this.originY = this._originalOriginY; | ||
this._originalOriginX = undefined; | ||
this._originalOriginY = undefined; | ||
} |
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.
Agreed
I would add normalizePoint
to the list.
I think it is used only by controls.
* @private | ||
* @returns {Point} dimensions | ||
*/ | ||
_getNonTransformedDimensions(): Point { |
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.
We should deprecate this method as well.
It is used only by Object._renderBackground
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.
And is weird in some context
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 getNonTransformedDimensions is not used anymore as before. is no more the base for getTransformed.
@ShaMan123 can you open 2 tasks with your comments? is not that i m lazy i m really short in time and i don't want to loose them. |
Motivation
Moving forward with the TS migration
Description
Really nothing else
Diffs are unreadable, i m sorry bewteen spacing and syntax changes git can't handle it.
Changes
All added lines are JSDOCS that were missing
BREAKING:
Gist
In Action