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.
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 |
| * 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); | ||
| // }, |
| 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; | ||
| }, |
| * 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.
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.
Agreed
I would add normalizePoint to the list.
I think it is used only by controls.
|
@ShaMan123 ok this is it. |
| * @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.
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.
Here is where I think of trying the skewY trick
| * @type Number | ||
| * @default 1 | ||
| */ | ||
| strokeWidth: number; |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
We should deprecate this method as well.
It is used only by Object._renderBackground
There was a problem hiding this comment.
And is weird in some context
There was a problem hiding this comment.
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