Use `popover` with Tooltip #307

clintcs · 2024-08-14T15:47:17Z

🚀 Description

Gave Tooltip a treatment similar to Menu, which let me get rid the last instance of setContainingBlock. It also fixed a couple outstanding bugs where Tooltip wasn't escaping its container.
Fixed an unrelated bug.
Swapped out the CSS arrow for SVGs to better match the mockups.
Tweaked spacing between the arrow and Tooltip's target.

📋 Checklist

I have read and followed the Contributing Guidelines.
I have added tests to cover new or updated functionality.
I have created or updated stories in Storybook to document the new functionality.
I have included a changeset with this Pull Request if it adds/updates/removes functionality for consumers.
I have scheduled a Design Review for these changes, if one is required.
I have followed the ARIA Authoring Practices Guide and/or met with the Accessibility Team to ensure this functionality is accessible.

🔬 How to Test

@mayuri-todkar tested this against a few applications (🙏). However, if you want to test a reproduction:

Navigate to the Modal (With Tertiary Content Icon) story.
Open Modal.
Hover over the information icon.
Make sure Tooltip breaks out of Modal.
Probably also worth navigating to Tooltip and checking each placement.

📸 Images/Videos of Functionality

Before

After

changeset-bot · 2024-08-14T15:47:20Z

🦋 Changeset detected

Latest commit: 4554cf0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@crowdstrike/glide-core	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-08-14T15:51:15Z

Preview URL

https://glide-core.crowdstrike-ux.workers.dev/switch-tooltip-to-popover

clintcs · 2024-08-15T12:56:17Z

src/tooltip.ts

+  @state()
+  private effectivePlacement: Placement = this.placement ?? 'bottom';

-  #arrowElementRef = createRef<HTMLDivElement>();


HTMLDivElement and HTMLSpanElement don't give us any special properties—and they make markup changes prone to unnecessary typechecking failures. I can't see a good use for them. Anyone else feel the same?

clintcs · 2024-08-15T13:02:25Z

src/tooltip.ts

-                  strategy: 'fixed',
                  middleware: [
-                    offset(6),
+                    offset(4),


From Design.

clintcs · 2024-08-15T13:05:00Z

src/tooltip.ts

-                    shift(),
-                    arrow({ element: arrowElement }),
+                    shift({
+                      limiter: limitShift({


Before

After

src/modal.tertiary-icon.ts

clintcs · 2024-08-15T15:11:17Z

src/tooltip.styles.ts

    }

+    :host {
+      display: inline-block;


Added this after debugging some tests. It cleans up our tests and is probably a better default for consumers who aren't using display: flex. Yeah?

Before

After

clintcs · 2024-08-15T15:17:11Z

src/tooltip.ts

-                top: arrowY === null ? '' : `${arrowY}px`,
-                right: '',
-                bottom: '',
-                [staticSide]: '-3px',


staticSide is no longer needed now that we're using an exact-size SVG instead of a box, which needed to be offset by half its height.

clintcs · 2024-08-15T16:34:30Z

src/tooltip.ts

  }

+  #hide() {
+    this.#cleanUpFloatingUi?.();


Halp! I can't seem to get coverage on this. Any ideas? It's called multiple times in tests. I'm not even sure what it means 😆.

The issue goes away if I switch #hide to private hide. Not great. But it's something. I'd leave a comment linking this comment.

I'll keep looking for a better solution.

Also does the trick:

if (this.#cleanUpFloatingUi) { this.#cleanUpFloatingUi(); }

Very silly. I'll push the above with a comment before merging.

If no one else has any ideas, I can pull this branch down tomorrow AM and give it a look

No worries. I suspect it's just an optional chaining bug deep in the bowels of Istanbul or wherever.

clintcs · 2024-08-15T16:57:22Z

src/tooltip.ts

+            data-test="arrow"
+            ${ref(this.#arrowElementRef)}
+          >
+            ${choose(this.effectivePlacement, [


choose is a thing!

ynotdraw

Thanks for doing this! This is great.

I noticed one small bug with the private-label stuff I think:

Screen.Recording.2024-08-15.at.1.56.06.PM.mov

ynotdraw · 2024-08-15T17:57:58Z

src/tooltip.styles.ts

 export default [
  css`
-    @keyframes animate-tooltip {
+    @keyframes tooltip {


I dig dropping the animate- prefix for keyframes 👍

Nice. I figured "animate" is implied—both here and when it's use with animation.

ynotdraw · 2024-08-15T17:58:13Z

src/tooltip.styles.ts

    }

+    :host {
+      display: inline-block;


ynotdraw · 2024-08-15T17:58:59Z

src/tooltip.styles.ts

+          A simple "transform" could replace Floating UI for the arrow if not for a Chrome 
+          bug with "popover" when "scale()" is animated on the popover or a container within 
+          it. With "transform" on the arrow, the arrow is initially offset by a couple pixels 
+          before landing in the correct position when the animation finishes. It only happens 
+          when the tooltip is left or right of its target.


Very annoying, thanks for the comments here

100%. What a pain this was.

ynotdraw · 2024-08-15T18:00:12Z

src/tooltip.styles.ts

+      --arrow-height: 0.375rem;
+      --arrow-width: 0.625rem;


Any reason not to duplicate these values in two spots or did you find value in bringing these up the chain to variables?

I originally used them in quite a few more places. But the only places they're used now are co-located. So I'll go ahead and duplicate them instead.

ynotdraw · 2024-08-15T19:05:27Z

src/tooltip.ts

+            data-test="arrow"
+            ${ref(this.#arrowElementRef)}
+          >
+            ${choose(this.effectivePlacement, [


ynotdraw · 2024-08-15T19:05:57Z

src/tooltip.ts

  }

+  #hide() {
+    this.#cleanUpFloatingUi?.();


If no one else has any ideas, I can pull this branch down tomorrow AM and give it a look

clintcs · 2024-08-15T19:16:58Z

I noticed one small bug with the private-label stuff I think:

Awesome find! Looking into it now.

danwenzel · 2024-08-15T20:58:37Z

src/tooltip.ts

 import { createRef, ref } from 'lit/directives/ref.js';
 import { customElement, property, state } from 'lit/decorators.js';
 import { ifDefined } from 'lit/directives/if-defined.js';
-import { offsetParent } from 'composed-offset-position';


You should be able to remove the composed-offset-position dependency from package.json now

Will do. Nice catch.

danwenzel

🎉

clintcs · 2024-08-15T21:24:46Z

src/tooltip.test.interactions.ts

I noticed one small bug with the private-label stuff I think:

@ynotdraw: Easy enough to fix. But this test should have caught it.

It didn't because I didn't add a await aTimeout(0) to it to await Floating UI—to give Tooltip a chance to become visible. And, of course, aTimeout doesn't work when timers are fake.

So I had work the timer stuff. I noticed a couple other tests were falsely passing and reworked those too. Instead of using fake timers, I gave the tests a way to override Tooltip's open and close delays.

I don't love the solution but couldn't find better way. More on why I went with data attributes here. Let me know what you think, all considered, or if you have other ideas.

Oh, also swapped out ARIA attribute selectors for data attribute selectors after coming across this, which I think I added.

Perhaps an example of why we should always use data attribute selectors in tests despite ARIA attribute selectors being in vogue because of Testing Library.

Nice work going through all of that! What you have here makes sense to me 👍

Perhaps an example of why we should always use data attribute selectors in tests despite ARIA attribute selectors being in vogue because of Testing Library.

Definitely. If I've got some time tomorrow I can write some thoughts up for CONTRIBUTING.md and update any remaining stragglers in the codebase if we'd like.

Nice. I've got a note too. I'll get to it if you don't.

ynotdraw

🎉 popover is so amazing!

ynotdraw · 2024-08-15T22:21:03Z

src/tooltip.ts

+    // The open and close delays are stored in data attributes so tests can
+    // configure them. Tests configure them, rather than using fake timers,
+    // because they need real timers so they can await Floating UI's setup.
+    //
+    // Conditionals here and in `#scheduleClose` based on `navigator.webdriver`
+    // would be a lot nicer. But the non-`navigator.webdriver` condition would
+    // never get hit in tests, so we'd fail to meet our coverage thresholds.


The other nice thing about these is that they're still private to the component due to the closed shadow root, so consumers have no way to adjust them (other than going through design and talking to us!). You love to see it!

Right, yeah. CSS variables would work here too. But, of course, they have the drawback of being public.

ynotdraw · 2024-08-15T22:25:01Z

src/tooltip.test.interactions.ts

Nice work going through all of that! What you have here makes sense to me 👍

Perhaps an example of why we should always use data attribute selectors in tests despite ARIA attribute selectors being in vogue because of Testing Library.

Definitely. If I've got some time tomorrow I can write some thoughts up for CONTRIBUTING.md and update any remaining stragglers in the codebase if we'd like.

clintcs · 2024-08-15T22:40:27Z

🎉 popover is so amazing!

No kidding. Many problems would be unfixable without it. I'm just going to throw popover on everything from now on. Not going to think twice.

clintcs changed the title ~~Switch tooltip to popover~~ Use popover with Tooltip Aug 14, 2024

clintcs force-pushed the switch-tooltip-to-popover branch 4 times, most recently from ba46889 to 85272af Compare August 15, 2024 02:28

clintcs commented Aug 15, 2024

View reviewed changes

clintcs force-pushed the switch-tooltip-to-popover branch from 85272af to 9b34405 Compare August 15, 2024 12:57

clintcs commented Aug 15, 2024

View reviewed changes

src/tooltip.ts Outdated

strategy: 'fixed',

middleware: [

offset(6),

offset(4),

Copy link

Collaborator Author

clintcs Aug 15, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

From Design.

clintcs commented Aug 15, 2024

View reviewed changes

clintcs force-pushed the switch-tooltip-to-popover branch 4 times, most recently from bcf6370 to 349d66e Compare August 15, 2024 14:39

clintcs commented Aug 15, 2024

View reviewed changes

src/modal.tertiary-icon.ts Outdated Show resolved Hide resolved

clintcs force-pushed the switch-tooltip-to-popover branch from 349d66e to 3721c4c Compare August 15, 2024 15:08

clintcs commented Aug 15, 2024

View reviewed changes

clintcs force-pushed the switch-tooltip-to-popover branch 5 times, most recently from 19ee16c to cb77a42 Compare August 15, 2024 16:16

clintcs commented Aug 15, 2024

View reviewed changes

clintcs force-pushed the switch-tooltip-to-popover branch 3 times, most recently from c67d9fb to 80f8a7b Compare August 15, 2024 16:55

clintcs commented Aug 15, 2024

View reviewed changes

Use popover with Tooltip

1f87efe

clintcs force-pushed the switch-tooltip-to-popover branch from 80f8a7b to 1f87efe Compare August 15, 2024 16:58

clintcs marked this pull request as ready for review August 15, 2024 17:01

clintcs requested review from danwenzel, dylankcrwd and ynotdraw as code owners August 15, 2024 17:01

ynotdraw reviewed Aug 15, 2024

View reviewed changes

danwenzel reviewed Aug 15, 2024

View reviewed changes

danwenzel approved these changes Aug 15, 2024

View reviewed changes

clintcs added 3 commits August 15, 2024 16:04

Work around test coverage gap

16f7b76

Rework tests and timers

d256117

Remove unused dependency

3307723

clintcs commented Aug 15, 2024

View reviewed changes

Remove Modal's hardcoded placement

4554cf0

ynotdraw approved these changes Aug 15, 2024

View reviewed changes

clintcs merged commit bb18490 into main Aug 15, 2024

clintcs deleted the switch-tooltip-to-popover branch August 15, 2024 22:47

github-actions bot mentioned this pull request Aug 15, 2024

Release Preview #293

Merged

Use popover with Tooltip #307

Use popover with Tooltip #307

Uh oh!

Conversation

clintcs commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Description

📋 Checklist

🔬 How to Test

📸 Images/Videos of Functionality

Before

After

Uh oh!

changeset-bot bot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Aug 14, 2024

Preview URL

Uh oh!

clintcs Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Before

After

Uh oh!

Uh oh!

clintcs Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Before

After

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ynotdraw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Use `popover` with Tooltip #307

Use `popover` with Tooltip #307

clintcs commented Aug 14, 2024 •

edited

Loading

changeset-bot bot commented Aug 14, 2024 •

edited

Loading

clintcs Aug 15, 2024 •

edited

Loading

clintcs Aug 15, 2024 •

edited

Loading

clintcs Aug 15, 2024 •

edited

Loading

clintcs Aug 15, 2024 •

edited

Loading

clintcs Aug 15, 2024 •

edited

Loading

clintcs Aug 15, 2024 •

edited

Loading

clintcs Aug 15, 2024 •

edited

Loading