Skip to content

Conversation

@spike-rabbit
Copy link
Member

The previous getOverlay function has some serious flaws. Replacing it with direct cdk usage.

See #808

Describe in detail what your merge request does and why. Add relevant
screenshots and reference related issues via Closes #XY or Related to #XY.


@github-actions
Copy link

github-actions bot commented Oct 7, 2025

@spike-rabbit spike-rabbit self-assigned this Oct 7, 2025
@spike-rabbit spike-rabbit force-pushed the refactor/tour/replace-overlay-helper branch 4 times, most recently from cef47bc to b90d900 Compare October 7, 2025 09:26
@spike-rabbit spike-rabbit requested a review from dr-itz October 7, 2025 09:42
@spike-rabbit
Copy link
Member Author

@dr-itz I removed some of the potential relevant cdk hacks you build, as I could not figure out why they are needed.
I hope this is due to the changes I did and potential cdk fixes. Please check if I missed something.

@dr-itz
Copy link
Contributor

dr-itz commented Oct 7, 2025

@dr-itz I removed some of the potential relevant cdk hacks you build, as I could not figure out why they are needed. I hope this is due to the changes I did and potential cdk fixes. Please check if I missed something.

well, put them back, it's completely broken now. see the demo with the open context menu. Clicking anywhere should have no effect at all, but now it does.

@spike-rabbit spike-rabbit force-pushed the refactor/tour/replace-overlay-helper branch from b90d900 to 0a64be4 Compare October 8, 2025 06:14
@spike-rabbit spike-rabbit requested a review from dr-itz October 8, 2025 06:29
@spike-rabbit spike-rabbit marked this pull request as ready for review October 8, 2025 06:29
@spike-rabbit spike-rabbit requested a review from a team as a code owner October 8, 2025 06:29
@spike-rabbit
Copy link
Member Author

Stuff is added back again.

@spike-rabbit spike-rabbit force-pushed the refactor/tour/replace-overlay-helper branch from 665e8d8 to ca2daf7 Compare October 8, 2025 12:31
Copy link
Contributor

@dr-itz dr-itz left a comment

Choose a reason for hiding this comment

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

one nit, otherwise seems to work ok

@spike-rabbit spike-rabbit force-pushed the refactor/tour/replace-overlay-helper branch from ca2daf7 to a15ad8f Compare November 6, 2025 17:52
@kfenner
Copy link
Member

kfenner commented Nov 7, 2025

@spike-rabbit So can we delete makeOverlay and makePositionStrategy?

In the issue you state:

In most cases, the overlay-helper function can just be replaced with direct cdk calls. For tooltips and popovers a a new contextual-overlay is potentially the best approach, as those components have basically the same requirements on terms of overlay location.

I don't understand this, can we now remove the whole overlay-helper.ts or what?

Otherwise I'm a bit confused, why are we adding 66 lines and only removing 29 if we can now use cdk?

@spike-rabbit
Copy link
Member Author

@kfenner yes, once I am done also extracting it for tooltip / popover, the whole overlay-helper code can be removed. But probably for v49 as we did not mark this as internal.

Copy link
Member

@kfenner kfenner left a comment

Choose a reason for hiding this comment

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

LGTM 👍

The previous `getOverlay` function has some serious flaws.
Replacing it with direct cdk usage.

See #808
@kfenner kfenner force-pushed the refactor/tour/replace-overlay-helper branch from a15ad8f to 54e641a Compare November 10, 2025 17:02
@kfenner kfenner enabled auto-merge (rebase) November 10, 2025 17:02
@github-actions
Copy link

Code Coverage

@kfenner kfenner merged commit befe77f into main Nov 10, 2025
11 checks passed
@kfenner kfenner deleted the refactor/tour/replace-overlay-helper branch November 10, 2025 17:13
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.

4 participants