Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

refactor: replace Popover with Tooltip in HelpTooltip#19635

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
aqandrew merged 36 commits intomainfromaqandrew/replace-popover-in-helptooltip
Sep 8, 2025

Conversation

aqandrew
Copy link
Contributor

for#19397

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

I opted forTooltip as a replacement because it's triggered on hover, unlike our newPopover which is triggered on click.

IdpOrgSyncPageView, TableColumnHelpTooltip, TemplateInsightsPage
@BrunoQuaresma
Copy link
Collaborator

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

aqandrew reacted with thumbs up emoji

@aqandrewaqandrew removed the request for review fromBrunoQuaresmaAugust 29, 2025 13:46
@aqandrewaqandrew marked this pull request as ready for reviewAugust 30, 2025 00:46
Copy link
Member

@ParkreinerParkreiner 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

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 haveHelpTooltip export a separateHelpTooltipTrigger 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 thatHelpTooltip just exportsTooltipTrigger under a different name – but I think that co-locating everything you need to wire up aHelpTooltip in one file is really important

Copy link
ContributorAuthor

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 theHelpTooltipTrigger component already exported byHelpTooltip renders a button with an SVG icon:

exportconstHelpTooltipTrigger=forwardRef<

which is currently instantiated in 16 places like this:

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

What do you think about renaming the existingHelpTooltipTrigger to something likeHelpTooltipIcon orHelpTooltipIconTrigger, so that we can export Radix'sTooltipTrigger asHelpTooltipTrigger 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

aqandrew reacted with thumbs up emoji
css,
typeInterpolation,
typeTheme,
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

aqandrew reacted with laugh emoji
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
ContributorAuthor

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 aHelpTooltipAction is clicked, we want to close the tooltip. By having the tooltip-closing logic built intoHelpTooltipAction by default, we could write handlers likeonClick={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

@ParkreinerParkreinerSep 3, 2025
edited
Loading

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 insidePopover.Root

I imagine that if we want to bake that behavior in, we'd basically need to make wrappers over all thePopover 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

aqandrew reacted with thumbs up emoji
exportconstHelpTooltip:FC<TooltipProps>=(props)=>{
return(
<TooltipProvider>
<TooltipdelayDuration={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
ContributorAuthor

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 theMUI popover, whichdeprecated/Popover uses, doesn't use any delay, and I don't think we've added any:

consthoverProps={
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
ContributorAuthor

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

@aqandrewaqandrew merged commitff18499 intomainSep 8, 2025
27 checks passed
@aqandrewaqandrew deleted the aqandrew/replace-popover-in-helptooltip branchSeptember 8, 2025 17:59
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsSep 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@ParkreinerParkreinerParkreiner approved these changes

@aslilacaslilacaslilac approved these changes

@BrunoQuaresmaBrunoQuaresmaAwaiting requested review from BrunoQuaresma

Assignees

@aqandrewaqandrew

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@aqandrew@BrunoQuaresma@aslilac@Parkreiner

[8]ページ先頭

©2009-2025 Movatter.jp