Skip to content

Conversation

aqandrew
Copy link
Contributor

for #19397

Currently there are 24 files that import bindings from the deprecated Popover component. One of those is HelpTooltip, which is instantiated in 24 other files. After this PR, the remaining files that import the deprecated Popover should be able to be migrated in just 1-2 more PRs. 🤞🏽

I opted for Tooltip as a replacement because it's triggered on hover, unlike our new Popover which is triggered on click.

@BrunoQuaresma
Copy link
Collaborator

Before I jump into review it, could you please fix the CI issues? So we can properly QA this.

@aqandrew aqandrew removed the request for review from BrunoQuaresma August 29, 2025 13:46
@aqandrew aqandrew marked this pull request as ready for review August 30, 2025 00:46
@aqandrew aqandrew requested a review from aslilac as a code owner August 30, 2025 00:46
@aqandrew aqandrew requested a review from Parkreiner as a code owner August 30, 2025 00:46
@aqandrew aqandrew requested a review from BrunoQuaresma August 30, 2025 00:46
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Everything looks pretty good to me. I had a few small nits, but nothing worth blocking over

Just let me know if you have any questions about my feedback

import {
HelpTooltip,
HelpTooltipContent,
HelpTooltipText,
HelpTooltipTitle,
} from "components/HelpTooltip/HelpTooltip";
import { Stack } from "components/Stack/Stack";
import { TooltipTrigger } from "components/Tooltip/Tooltip";
Copy link
Member

Choose a reason for hiding this comment

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

Stuff like this is making me worry we're not closing the loop on the abstraction all the way

I think my preference would be to have HelpTooltip export a separate HelpTooltipTrigger component, so that in the future, people don't need to go rifling around for the right component to get the help tooltip wired up. We could even make it so that HelpTooltip just exports TooltipTrigger under a different name – but I think that co-locating everything you need to wire up a HelpTooltip in one file is really important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tricky thing is that the HelpTooltipTrigger component already exported by HelpTooltip renders a button with an SVG icon:

export const HelpTooltipTrigger = forwardRef<

which is currently instantiated in 16 places like this:

<HelpTooltipTrigger size="small" />

So my thought with using TooltipTrigger here is that you can insert whatever elements you want to act as a trigger (like in AgentLatency we're using a span instead of a button). Looking at it again, it is clunky.

What do you think about renaming the existing HelpTooltipTrigger to something like HelpTooltipIcon or HelpTooltipIconTrigger, so that we can export Radix's TooltipTrigger as HelpTooltipTrigger like you suggested?

Copy link
Member

Choose a reason for hiding this comment

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

I think I like the renaming approach better. I'm not even fully sure about having a version of the trigger that has an icon pre-baked, but I don't think it's all that egregious, and I imagine it'd be pretty easy to change it down the line

@@ -3,18 +3,17 @@ import {
css,
type Interpolation,
type Theme,
useTheme,
Copy link
Member

Choose a reason for hiding this comment

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

🙏

Comment on lines +23 to +25
expect(screen.getByRole("tooltip")).toHaveTextContent(
meta.args.message,
),
Copy link
Member

Choose a reason for hiding this comment

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

Big fan of the more accessible selectors

Comment on lines 72 to 76
onClick={() => {
onUpdate();
// TODO can we move this tooltip-closing logic to the definition of HelpTooltipAction?
setIsOpen(false);
}}
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to get some clarity on this comment: was this meant more in the sense of "I'm not sure if it's a good idea to do this", or "I feel like this is a good idea, but I'm not 100% sure how to do it"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit of both, but more the latter. I think whenever a HelpTooltipAction is clicked, we want to close the tooltip. By having the tooltip-closing logic built into HelpTooltipAction by default, we could write handlers like onClick={onUpdate} here + wherever else we want to execute a function when closing a tooltip.

Since Radix doesn't export a component like <Popover.Close />, it seems the only way to do this is with a state hook. Is it a good idea to make every tooltip a controlled component? I'm not sure

Copy link
Member

@Parkreiner Parkreiner Sep 3, 2025

Choose a reason for hiding this comment

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

Yeah, looking at Radix's API docs, I don't see anything that gives direct access to the state that's being contained inside Popover.Root

I imagine that if we want to bake that behavior in, we'd basically need to make wrappers over all the Popover components, and then wire up our own custom context (on top of the one already used by Radix) to control the closing state. I don't think that's too bad, but I also feel like Radix could've easily given a way to trigger that behavior if they felt like it was a common enough usage pattern

Unless we have a bunch of other components wiring that behavior up already, I feel like we can leave things be and wait for more examples to emerge

export const HelpTooltip: FC<TooltipProps> = (props) => {
return (
<TooltipProvider>
<Tooltip delayDuration={0} {...props} />
Copy link
Member

Choose a reason for hiding this comment

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

I think 0 is probably too fast. what's the default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 700 which feels very long.

Worth mentioning that the MUI popover, which deprecated/Popover uses, doesn't use any delay, and I don't think we've added any:

const hoverProps = {
onPointerEnter: (event: PointerEvent<HTMLElement>) => {
popover.setOpen(true);

so I used a delay of 0 to keep consistency with our current UX

Copy link
Member

Choose a reason for hiding this comment

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

could we set it to like, 200? I feel like 0 is intense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, 0 is exactly as intense as prod is rn:

Screen.Recording.2025-09-04.at.4.05.34.PM.mov

I'm so used to 0 delay that 200 feels sluggish to me:

Screen.Recording.2025-09-04.at.4.03.46.PM.mov

@aqandrew aqandrew merged commit ff18499 into main Sep 8, 2025
32 checks passed
@aqandrew aqandrew deleted the aqandrew/replace-popover-in-helptooltip branch September 8, 2025 17:59
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants