-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… useEffect with useEffectEvent
buenos-nachos
left a comment
There was a problem hiding this 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
| expect(await screen.findByRole("tooltip")).toHaveTextContent( | ||
| /Latency not available/i, | ||
| ); |
There was a problem hiding this comment.
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 that findByRole 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. The findByRole 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that about findByRole, 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(() => { |
There was a problem hiding this comment.
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, with useEffectEvent, you want to do two things:
- Identify the reactive dependencies – the things that should be treated as synchronization cues between React and an external system. These should stay out of the
useEffectEventcallback, and should still be included in theuseEffectdependency array - 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 the
useEffectEventcallback
There was a problem hiding this comment.
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, we should include the effect event callback in the useEffect dependency array for correctness, but the value will never trigger re-renders by itself. But once we upgrade, we should be able to remove useEffectEvent from all dependency arrays (assuming Biome updates their rules like ESLint did)
| open = false, | ||
| ...contentProps | ||
| }) => { | ||
| const [isOpen, setIsOpen] = useState(open); |
There was a problem hiding this comment.
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 the open prop ever changes during re-renders, then we won't ever acknowledge it, because we'll be driving everything via the isOpen state instead
I think this will work
const MiniTooltip: FC<MiniTooltipProps> = ({
// No default assignment
open,
// Also destructure onOpenChange, even if it's not defined
onOpenChange,
...contentProps
}) => {
const [isOpen, setIsOpen] = useState(false);
const activeIsOpen = 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 behind
const handleOpenChange = (newOpen: boolean) => {
setIsOpen(newOpen);
onOpenChange?.(newOpen);
};…eplacing useEffect with useEffectEvent" This reverts commit 9398e0d.
…20849) for #19974 Redo of #20027, this time splitting it into multiple PRs + using our existing `Tooltip` component instead of creating a new component (see below). 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 Storybook decorators, so that we don't have to render a `TooltipProvider` for every tooltip instance. Removing redundant `TooltipProvider`s will be another separate PR~~ <- this was done by #20869
closes #19974