-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fixed Rect results for a sequenced range #7840
Conversation
function copyRangeRectProperties( rect, source ) { | ||
const rangeRects = Rect.getDomRangeRects( source ); | ||
const combinedRect = rangeRects[ 0 ]; | ||
|
||
for ( let i = 1; i < rangeRects.length; i++ ) { | ||
combinedRect.right = Math.max( combinedRect.right, rangeRects[ i ].right ); | ||
combinedRect.left = Math.min( combinedRect.left, rangeRects[ i ].left ); | ||
combinedRect.bottom = Math.max( combinedRect.bottom, rangeRects[ i ].bottom ); | ||
combinedRect.top = Math.min( combinedRect.top, rangeRects[ i ].top ); | ||
} | ||
|
||
combinedRect.width = combinedRect.right - combinedRect.left; | ||
combinedRect.height = combinedRect.bottom - combinedRect.top; | ||
|
||
copyRectProperties( rect, combinedRect ); | ||
} |
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 the table package we have a helper function that is very similar (the same purpose but different approach). Maybe we could unify those to one helper function?
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.
Good idea, added #7858 that will also be closed by this PR.
I had a small problem with the naming, first wanted to make it a Rect#expand()
member, but the name didn't feel right. Ended up with static Rect.getBoundingRect()
which feels just right 👌
Also I simplified mentioned contextualballoon logic a little bit as the way how createBoundingRect
internal logic was implemented in the getBalloonCellPositionData
function was a little 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.
I guess you mean Rect
not Range
😉
…turns a `Rect` instance containing all other rectangles.
…on toolbar position.
Suggested merge commit message (convention)
Fix (utils):
Rect
utility returns wrong sizes in case of a sequenced range. Closes #7838.Feature (utils): Introduced the
Rect#getBoundingRect()
method that returns aRect
instance containing all other rectangles. Closes #7858.Additional information
Initially I was even tempted to simplify it and use
Range.getBoundingClientRect()
to do the computation for us, but then I saw a plethora of issues that this method not always give reliable results: