Skip to content
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

Fix infinite resizing issue in overlay positioning #6312

Merged
merged 3 commits into from
May 3, 2024
Merged

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented May 1, 2024

Closes #6241

@rspbot
Copy link

rspbot commented May 1, 2024

@rspbot
Copy link

rspbot commented May 2, 2024

@@ -199,123 +199,123 @@ describe('calculatePosition', function () {
const testCases = [
{
placement: 'left',
noOffset: [50, 200, undefined, 196, 350],
offsetBefore: [-200, 50, undefined, 50, 500],
noOffset: [50, 200, undefined, 100, 350],
Copy link
Member

@LFDanLu LFDanLu May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For others who may be looking at the tests and wondering what these number mean:

  • first value is either position: left of the overlay or a value used to calculate the corresponding position: right (can just think of it as position: left still)
  • second value is either position: top of the overlay or a value used to calculate the corresponding position: bottom (can just think of it as position: top still)
  • third value is arrowOffsetLeft, aka the number of px that the arrow is shifted horizontally with respect to the left edge of the overlay if any (only applies for top/bottom placements)
  • fourth value is arrowOffsetTop, aka the number of px that the arrow is shifted with respect to the top edge of the overlay (only applies for left/right placements)
  • the last value is the overlay's calculated max height, essentially the amount of space that the overlay can grow before hitting the boundary's edge

Translation of these values for this test case (placement: left + noOffset):

        checkPosition(
          placement, getTargetDimension({left: 250, top: 250}), testCase.noOffset
        );

becomes:

  • container is 600x600 with a padding of 50 so valid render area is a rect of {left: 50, right: 550, top: 50, bottom: 550}
  • button is placed at (250, 250) with a height of (100, 100) => {left: 250, right: 350, top: 250, bottom: 350}. Note that this (250, 250) placement combined with specified height/width essentially places the button in the exact middle of the container
  • overlay is 200x200 and has a calculated position of {left: 50, right: 250, top: 200, bottom: 400} and a maxHeight of 350 since it is 550 (boundary bottom) - 200 (top of overlay)
  • arrow size is 8, and an arrowOffsetTop of 100 aka it is placed 100px from the top edge of the overlay which is the center of overlay and matches the center of the button: 200 (overlay top) + 100 (arrow offset) = 300 (center of button)

@LFDanLu
Copy link
Member

LFDanLu commented May 2, 2024

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked chrome and FF, looking good

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, also stepped through most of the test logic and the numbers seem to check out

@rspbot
Copy link

rspbot commented May 2, 2024

@rspbot
Copy link

rspbot commented May 2, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@LFDanLu LFDanLu merged commit 800f9a0 into main May 3, 2024
25 checks passed
@LFDanLu LFDanLu deleted the overlay-max-height branch May 3, 2024 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ComboBox list items not scrolling
4 participants