-
Notifications
You must be signed in to change notification settings - Fork 1k
refactor: migrate CopyableValue from MUI to Radix UI #20261
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
base: main
Are you sure you want to change the base?
Conversation
Replace MUI Tooltip with Radix Tooltip components and migrate Emotion CSS to Tailwind utility classes. This is part of the ongoing effort to standardize on Radix UI/shadcn components and Tailwind CSS. Changes: - Replace @mui/material/Tooltip with components/Tooltip/Tooltip (Radix) - Remove unused PopperProps (not used anywhere in codebase) - Rename placement prop to side to match Radix API - Replace Emotion css={{ cursor: "pointer" }} with Tailwind cursor-pointer class - Update single call site in IconsPage.tsx (placement → side) - Wrap in TooltipProvider as required by Radix Technical details: - Radix uses Provider/Tooltip/Trigger/Content pattern (more verbose but flexible) - Maintained backward compatibility for all other call sites (6 files) - No functional changes to component behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
if (showCopiedSuccess !== prevCopiedRef.current) { | ||
prevCopiedRef.current = showCopiedSuccess; | ||
|
||
if (showCopiedSuccess) { | ||
setShowCopiedText(true); | ||
} else { | ||
setTooltipOpen(false); | ||
clearTimeout(timeoutRef.current); | ||
// showCopiedText should reset after tooltip is closed to avoid a flash of the text | ||
timeoutRef.current = window.setTimeout( | ||
() => setShowCopiedText(false), | ||
100, | ||
); | ||
} | ||
} |
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.
if the idea is to reset things after the tooltip is hidden, this should be in an onOpenChange
handler, not inline during render.
if (showCopiedSuccess) { | ||
setShowCopiedText(true); |
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.
why have both showCopiedSuccess
and showCopiedText
? this just feels like duplicated state with messy synchronization code
spacing={1} | ||
justifyContent="center" | ||
<div | ||
css={{ marginTop: 32 }} |
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.
can we convert this too while we're at it?
<Stack alignItems="center" css={{ margin: 12 }}> | ||
<CopyableValue key={icon.url} value={icon.url}> | ||
<div | ||
css={{ margin: 12 }} |
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.
and here
Summary
Migrates the
CopyableValue
component from Material-UI Tooltip to Radix UI Tooltip.Changes
CopyableValue Component
PopperProps
(never used in any call site)placement
→side
(Radix naming convention)Minimal API surface change:
placement
prop renamed toside
(only affects 1 call site - already updated)PopperProps
removed (was never used)