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

ui: Move tooltips to overlay #995

Merged
merged 3 commits into from
Jan 24, 2024
Merged

ui: Move tooltips to overlay #995

merged 3 commits into from
Jan 24, 2024

Conversation

backspace
Copy link
Contributor

@backspace backspace commented Jan 24, 2024

This fixes the truncation in Safari seen in the top window here:

screencast 2024-01-22 13-49-59

The Tooltip component in boxel-ui finds or inserts an overlay element for tooltips to be inserted into. To make tests that make assertions about tooltips the overlay is inserted on owner.rootElement despite that being undocumented API.

This fixes a truncation in Safari.
@backspace backspace added the bug Something isn't working label Jan 24, 2024
@backspace backspace self-assigned this Jan 24, 2024
Copy link
Contributor

@lukemelia lukemelia left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link

github-actions bot commented Jan 24, 2024

Test Results

490 tests  ±0   486 ✔️ ±0   6m 38s ⏱️ +9s
    1 suites ±0       4 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 33d5c1c. ± Comparison against base commit 76fbc0b.

♻️ This comment has been updated with latest results.

@backspace
Copy link
Contributor Author

backspace commented Jan 24, 2024

Unfortunately this breaks some test expectations. The tooltips are indeed showing but even when I adjust the assertions the elements can’t be found, presumably because it’s outside the test container.

Boxel Tests 2024-01-24 12-14-15

I’m unsure what best practice is here for finding the right container, part of the difficulty is because Tooltip is in boxel-ui but being consumed in host. That’s what I’ll look into now.

ETA: owner.rootElement works despite being undocumented…?

@backspace backspace requested a review from a team January 24, 2024 19:18
@backspace backspace merged commit 92bbb26 into main Jan 24, 2024
22 checks passed
@delete-merged-branch delete-merged-branch bot deleted the ui/tooltip-truncation-cs-6521 branch January 24, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants