-
-
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): Group types #8807
chore(TS): Group types #8807
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.
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.
1 thing
src/shapes/Group.ts
Outdated
width: w, | ||
height: h, |
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.
might be this is an unwanted change?
bbox.width/height could have been undefined if group has no objects and then _getTransformedDimensions
would use this.width/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 don't even know how we eneded up pass width and height to _getTransformedDimensions
, it accepted skewX and skeW for the only reason to cheat during skew control support, but shouldn't take anything to be honest.
Regarding your question:
const width = hasWidth ? this.width : w,
height = hasHeight ? this.height : h,
would using width and height local vars effectively sort the issue?
If bbox does not return width/height ( but why???) it would have this.width and 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.
In #8767 I changed this piece of logic. And I want to change a bit more so we shouldn't dig into this here IMO.
The only reason it wouldn't return values for getObjectsBoundingBox
is if group has no children.
Really, this part is not good enough in general.
width/height from props should take affect only if the layout type is fixed
. All other types should ignore it.
The center point however is affected by left/top for all cases.
What I want to do is first set the center point and after that preform the initial layout.
I think we can move forward with this even if we commit a regression since obviously we need to look into this anyways and the BBox approach should reduce the layout logic by half and enable 10 times more possibilities.
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.
Ok so i ll remove this change here.
Groups has still a bunch of type violations all around the file, i ll put back the |
Motivation
Description
types, props and some patching
Changes
Gist
In Action