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 MUI Tooltip with MiniTooltip#20027

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

Closed
aqandrew wants to merge16 commits intomainfromaqandrew/replace-mui-tooltip

Conversation

@aqandrew
Copy link
Contributor

@aqandrewaqandrew commentedSep 30, 2025
edited
Loading

closes#19974

Copy link
Contributor

@buenos-nachosbuenos-nachos left a comment

Choose a reason for hiding this comment

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

Things look good so far. Just had a few comments

Comment on lines 49 to 51
expect(awaitscreen.findByRole("tooltip")).toHaveTextContent(
/Latencynotavailable/i,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is partly personal taste, but I just want to make sure you know thatfindByRole basically functions as a form of assertion by itself, because if the selector fails to find the element in some period of time, the test will fail

But I feel like here, this has a problem of causing flakes in the future, if we start needing to add more tooltip content. ThefindByRole call will find the first tooltip it can, and then we assert that it has specific content. So if a new tooltip gets introduced, and it happens to be earlier in the DOM tree, this test will start breaking

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Didn't know that aboutfindByRole, thank you. I could've sworn that the Radix tooltip's DOM structure made the original assertion fail, but I just reverted it back and it seems to be working again.

const[selectedPresetIndex,setSelectedPresetIndex]=useState(0);
// Build options and keep default label/value in sync
useEffect(()=>{
useEffectEvent(()=>{
Copy link
Contributor

@buenos-nachosbuenos-nachosOct 15, 2025
edited
Loading

Choose a reason for hiding this comment

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

I was a bit rushed, so I didn't go through the full file to see what might be causing some of the infinite renders, but basically, withuseEffectEvent, you want to do two things:

  1. Identify the reactive dependencies – the things thatshould be treated as synchronization cues between React and an external system. These should stay out of theuseEffectEvent callback, and should still be included in theuseEffect dependency array
  2. Identify the "incidental" dependencies – the things that you need to perform all necessary calculations, but that shouldn't be treated as synchronization cues themselves. These should all go in theuseEffectEvent callback

Copy link
Contributor

@buenos-nachosbuenos-nachosOct 15, 2025
edited
Loading

Choose a reason for hiding this comment

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

Until we upgrade to React 19.2, weshould include the effect event callback in theuseEffect dependency array for correctness, but the value will never trigger re-renders by itself. But once we upgrade, we should be able to removeuseEffectEvent from all dependency arrays (assuming Biome updates their rules like ESLint did)

open=false,
...contentProps
})=>{
const[isOpen,setIsOpen]=useState(open);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to have state here, but the current implementation will cause bugs. If theopen prop ever changes during re-renders, then we won't ever acknowledge it, because we'll be driving everything via theisOpen state instead

I think this will work

constMiniTooltip:FC<MiniTooltipProps>=({// No default assignmentopen,// Also destructure onOpenChange, even if it's not defined  onOpenChange,...contentProps})=>{const[isOpen,setIsOpen]=useState(false);constactiveIsOpen=open??isOpen;// Sync state change to both so that if the component ever flips from// controlled to uncontrolled, the other piece of state won't be lagging behindconsthandleOpenChange=(newOpen:boolean)=>{setIsOpen(newOpen);onOpenChange?.(newOpen);};

@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelOct 24, 2025
aqandrew added a commit that referenced this pull requestNov 25, 2025
…20849)for#19974 Redo of#20027, this time splitting it into multiple PRs + using ourexisting `Tooltip` component instead of creating a new component (seebelow). This PR covers the most basic usage of the MUI Tooltip, i.e.,the tooltip content is a string literal.~~Adds a global `TooltipProvider` to `AppProviders` and our Storybookdecorators, so that we don't have to render a `TooltipProvider` forevery tooltip instance. Removing redundant `TooltipProvider`s will beanother separate PR~~ <- this was done by#20869
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@buenos-nachosbuenos-nachosbuenos-nachos left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@aqandrewaqandrew

Labels

staleThis issue is like stale bread.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

replace MUI tooltip component with newTooltip

3 participants

@aqandrew@buenos-nachos

[8]ページ先頭

©2009-2025 Movatter.jp