-
Notifications
You must be signed in to change notification settings - Fork 1.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
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) { | ||
| 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
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.
Yeah, I'm struggling to wrap my head around what the code is trying to solve that isn't handled by using showCopiedSuccess directly.
The hook already has timeouts built in (with cleanup). Is the goal to expose a different timeout value to the rest of the component? Because if so, it feels like the better change is to update useClipboard so that the timeout is parameterized
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.
My original goal was to make it so that the tooltip is removed after showCopiedSuccess returns to false as it felt like a better experience when I originally looked at this. I think after thinking over this again. The existing functionality makes sense and I will remove all of this extra logic.
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.
The current code breaks a bunch of React rules, and I don't see any benefits to it. I see a lot of these changes causing bugs down the line
| 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.
Yeah, I'm struggling to wrap my head around what the code is trying to solve that isn't handled by using showCopiedSuccess directly.
The hook already has timeouts built in (with cleanup). Is the goal to expose a different timeout value to the rest of the component? Because if so, it feels like the better change is to update useClipboard so that the timeout is parameterized
|
@Parkreiner @aslilac Removed the code where there was concerns and fixed issues with keyboard handling as well. The component should have now have parity with the MUI implementation. |
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.
We're making progress, but I don't think we're there just yet
| {...attrs} | ||
| {...clickableProps} |
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 gong to cause prop shadowing for all the props that clickableProps currently has (as in, if a consumer passes in any of tabIndex, role, onClick, onKeyDown or onKeyUp, they'll get dropped in favor of the clickableProps versions)
I feel like we want to be more finer-grained with how we wire everything together (this just illustrates the main idea, and doesn't account for all the other code comments I made):
export const CopyableValue: FC<CopyableValueProps> = ({
value,
side = "bottom",
children,
className,
role,
tabIndex,
onClick,
onKeyDown,
onKeyUp,
...attrs
}) => {
const { showCopiedSuccess, copyToClipboard } = useClipboard();
const [tooltipOpen, setTooltipOpen] = useState(false);
const [isFocused, setIsFocused] = useState(false);
const clickableProps = useClickable<HTMLSpanElement>(() => {
copyToClipboard(value);
setTooltipOpen(true);
});
return (
<TooltipProvider delayDuration={100}>
<Tooltip
open={tooltipOpen}
onOpenChange={(next) => {
if (!next && isFocused) return;
setTooltipOpen(next);
}}
>
<TooltipTrigger asChild>
<span
ref={clickableProps.ref}
{...attrs}
className={cn("cursor-pointer", className)}
role={role ?? clickableProps.role}
tabIndex={tabIndex ?? clickableProps.tabIndex}
onClick={(event) => {
clickableProps.onClick(event);
onClick?.(event);
}}
onKeyDown={(event) => {
clickableProps.onKeyDown(event);
onKeyDown?.(event);
}}
onKeyUp={(event) => {
clickableProps.onKeyUp(event);
onKeyUp?.(event);
}}
onMouseEnter={() => {
setIsFocused(true);
setTooltipOpen(true);
}}
onMouseLeave={() => {
setTooltipOpen(false);
}}
onFocus={() => {
setIsFocused(true);
}}
onBlur={() => {
setTooltipOpen(false);
}}
>
{children}
</span>
</TooltipTrigger>
<TooltipContent side={side}>
{showCopiedSuccess ? "Copied!" : "Click to copy"}
</TooltipContent>
</Tooltip>
</TooltipProvider>
);
};| }) => { | ||
| const { showCopiedSuccess, copyToClipboard } = useClipboard(); | ||
| const [tooltipOpen, setTooltipOpen] = useState(false); | ||
| const [isFocused, setIsFocused] = useState(false); |
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.
What is our goal with the focus state? Because right now, it looks like we're flipping the state within React, but we're not actually changing the focus behavior on the actual HTML element
The main area where I'm worried about drift between React and the underlying HTML is the onMouseEnter prop setting isFocused to true
| onFocus={() => { | ||
| setIsFocused(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.
Do we also want to open the tooltip here?
| // Always keep the tooltip open when in focus to handle issues when onOpenChange is unexpectedly false | ||
| if (!next && isFocused) return; |
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.
What situations were you running into that was causing this problem? I know that the libraries can get a little unruly and sometimes need these hacks, but I'm wondering if there's a chance we could remove the need for this
| <CopyableValue key={icon.url} value={icon.url} placement="bottom"> | ||
| <Stack alignItems="center" css={{ margin: 12 }}> | ||
| <CopyableValue key={icon.url} value={icon.url}> | ||
| <div className="flex flex-col gap-4 items-center max-w-full m-3"> |
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.
| <div className="flex flex-col gap-4 items-center max-w-full m-3"> | |
| <div className="flex flex-col gap-4 items-center max-w-full p-3"> |
can we do padding instead if it looks the same?
| <TooltipProvider delayDuration={100}> | ||
| <Tooltip | ||
| open={tooltipOpen} | ||
| onOpenChange={(next) => { |
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.
| onOpenChange={(next) => { | |
| onOpenChange={(shouldBeOpen) => { |
the name next just makes this really hard to reason about for me. if I substitute it for something like this in my head it makes it clearer.
Summary
Migrates the
CopyableValuecomponent 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:
placementprop renamed toside(only affects 1 call site - already updated)PopperPropsremoved (was never used)