[Design System] Adds MUI Tooltip - #74565
Conversation
🖼️ Storybook Visual Comparison ReportA difference was found in our Storybook front-end visual comparison testing against the Remediation steps:
|
There was a problem hiding this comment.
Pull request overview
Adds an MUI-backed design-system tooltip while retaining legacy tooltip support.
Changes:
- Adds tooltip sizing, theming, and keyboard-only behavior.
- Registers MUI theme overrides.
- Adds stories, documentation, and tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tooltip/Tooltip.tsx |
Implements the MUI wrapper. |
tooltip/stories/Tooltip.story.tsx |
Adds usage stories. |
tooltip/README.md |
Documents migration and API. |
tooltip/index.ts |
Exports new and legacy tooltips. |
tooltip/__tests__/WithTooltip.test.tsx |
Preserves legacy tests. |
tooltip/__tests__/Tooltip.test.tsx |
Tests the MUI implementation. |
styleOverrides/tooltip.ts |
Defines tooltip defaults and styles. |
styleOverrides/index.ts |
Registers tooltip overrides. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (5)
frontend/packages/component-library/src/tooltip/stories/Tooltip.story.tsx:59
- Move this static placement spacing into the story SCSS module rather than adding another inline style.
<div style={{padding: '4rem 0'}}>
frontend/packages/component-library/src/tooltip/stories/Tooltip.story.tsx:82
- This themed-container styling is also static and must use an SCSS module under the design-system styling rule instead of an inline style object.
style={{
display: 'flex',
gap: '2rem',
padding: '1.5rem',
borderRadius: '0.5rem',
frontend/packages/component-library/src/tooltip/README.md:65
- This says the behavior uses two props, but
KEYBOARD_ONLY_PROPSalso forcesdisableFocusListenertofalse. Document that override because it changes the result when a caller suppliesdisableFocusListener.
The implementation is two props, because MUI's `Tooltip` already checks
`:focus-visible` before opening on focus. Switching off the hover and touch
listeners is the whole behavior: a click that moves focus to the trigger leaves
the tooltip shut, a Tab to it opens it. Escape closes it either way.
frontend/packages/component-library/src/tooltip/stories/Tooltip.story.tsx:23
- Move this static row layout into a story SCSS module. The design-system guidance requires SCSS modules and prohibits inline styles.
This issue also appears in the following locations of the same file:
- line 59
- line 78
<div style={{display: 'flex', gap: '2rem', flexWrap: 'wrap'}}>{children}</div>
frontend/packages/component-library/src/tooltip/tests/Tooltip.test.tsx:137
user.hoverreturns after dispatching the events, but MUI's defaultenterDelayis 100 ms. This negative assertion can therefore pass before an accidentally enabled hover listener opens the tooltip. Remove that blind spot by setting the delay to zero in this test.
renderKeyboardOnly();
await user.hover(screen.getByRole('button'));
// No enter delay by default, so nothing is pending.
expect(screen.queryByRole('tooltip')).not.toBeInTheDocument();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
frontend/packages/component-library/src/tooltip/Tooltip.tsx:75
- MUI accepts
0as a valid tooltip title, but this truthiness check drops the requested leading icon for that title. Match MUI's empty-title semantics sotitle={0}still renders both the icon and title.
iconName && title ? (
frontend/packages/component-library/src/tooltip/tests/Tooltip.test.tsx:19
- This replacement removes the only unit coverage for the still-exported and widely used
WithTooltip: hover display, imperative hiding, and tail toggling are no longer tested. Preserve those tests in a separate legacyWithTooltiptest file while adding this MUI suite.
describe('Design System - Tooltip (MUI)', () => {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
frontend/packages/component-library/src/tooltip/Tooltip.tsx:98
- MUI treats numeric
0as a valid, visible title, but this truthiness check drops the requested leading icon for that title while the tooltip itself still renders. Match MUI's empty-title check soiconNamealso works withtitle={0}.
iconName && title ? (
frontend/packages/component-library/src/tooltip/Tooltip.tsx:85
- MUI 7.3.5 still accepts
componentsProps.tooltip. Because this wrapper always suppliesslotProps.tooltip, MUI ignores that deprecated fallback, so a caller's tooltip class, style, handlers, or ref are silently lost despiteCdoTooltipPropsextending all MUI props. UsecomponentsProps.tooltipwhen the caller did not provideslotProps.tooltip.
const callerTooltipProps = slotProps?.tooltip;
|
Tyy!! I'm on mobile but visually this looks great! The main thing that could be simplified is that the new CADS tooltip specs intentionally drop size options for the tooltip and stays with one fixed size option. The other is that for things like border radius and shadows, it’d be great if we could use the new CADS variables/values (these should be in production already and naming should match Figma!). |
It looks like the shadows aren't tokenized yet- do you want me to do so? They're hard coded in toast.module.scss. And also good catch- will remove sizes! |
Oo good catch, thought we had them in there! Just opened a pr to add (cc @levadadenys)! |
moshebaricdo
left a comment
moshebaricdo
left a comment
There was a problem hiding this comment.
Tysm!! Lgtm visually with some tweaking to get it to visual parity with the spec, added a link to a CSS module in the comments that'll hopefully help!
Updating Sketch Lab to use design system should be seen as a quick/free win, not a blocker. cc @moshebaricdo please correct me if I'm wrong from design perspective |
It isn't a blocker, to be clear! Claude is just mentioning a quick line of code required now to keep the migration as a fast follow. But happy to migrate those now if you'd rather. Not a big deal either way |
levadadenys
left a comment
levadadenys
left a comment
There was a problem hiding this comment.
This is really thorough work — the README in particular anticipates most of what I
came here to ask, which made reviewing it a pleasure. I want to push back on one
architectural choice, though, and I think the README's own reasoning is the best
place to start.
The ask: drop Tooltip.tsx and make this migration theme-only, the way
Button, IconButton and Breadcrumbs went — style overrides plus MUI prop
augmentation, and call sites import {Tooltip} from '@mui/material' directly.
The README's argument for the wrapper is that "keyboardOnly and iconName are
behavior, and a theme override can only supply styles." That's true as far as it
goes, but it turns out the theme can carry more than styles, and the two
behaviors that are left don't need a component.
I wrote a proof of concept to check rather than guess — 12 passing tests against
MUI 7.3.5 and the real CdoTheme, no source changes, sitting on staging rather
than your branch so it doesn't depend on anything of yours:
denys/poc/tooltip-theme-only.
Check it out and run yarn vitest --run src/tooltip/__tests__/PocThemeOnly.test.tsx
from frontend/packages/component-library.
What moves into the theme
Tooltip.tsx does |
Theme mechanism | PoC |
|---|---|---|
data-size attribute → sized CSS |
styleOverrides.tooltip: ({theme, ownerState}) => …ownerState.size — the breadcrumbs.ts pattern |
✅ size="s" → body3 |
| arrow sizing per size | same, in the arrow slot — ownerState reaches it too |
✅ |
hasCaret default true |
defaultProps: {arrow: true} |
✅ |
describeChild default true |
defaultProps: {describeChild: true} |
✅ aria-describedby, no aria-label |
data-cdo-tooltip marker |
defaultProps.slotProps.tooltip accepts an ownerState function, so attributes are theme-plumbable too |
✅ |
| RTL placement mirroring | createTheme({direction: 'rtl'}) — MUI wires RtlProvider off theme.direction |
✅ bottom-start → data-popper-placement="bottom-end" |
keyboardOnly |
disableHoverListener + disableTouchListener at the call site |
✅ hover shut, Tab opens |
iconName |
title={<><Icon/>text</>}; the theme still sizes it via & i |
call site |
The size leak is real but narrower than documented
Your read of the mechanism is right — MUI 7 spreads ...other onto the cloned
child at Tooltip.js:553-556. The consequences are milder than "an MUI trigger
such as IconButton has its own size, which ours would override and shrink":
- Plain DOM trigger: React refuses to render an unknown
sizeon a
<button>, so nothing leaks at all. - MUI child that sets its own size:
...children.propsis spread after
...other, so the child wins. Every tooltip-wrappedIconButtonin Sketch Lab
already passes an explicitsize. - MUI child with no size: this is the one that bites — you get
MuiIconButton-sizeS, a dead class, and the button silently loses
sizeMedium.
And it's fully defusable without a wrapper: name the augmented prop data-size
rather than size and read ownerState['data-size'] in the override. The leak
becomes a harmless data attribute on the trigger, the child keeps its own size,
and the CSS you've already written doesn't change. Last test in the PoC.
The one thing I can't settle from the code
The data-cdo-tooltip marker is the only load-bearing reason left for the
wrapper, and it's a product question, not a technical one. It's there so the
override won't restyle the five Sketch Lab files already using bare MUI
Tooltip — but those are all plain <Tooltip title="Duplicate" placement="top">
sitting on MUI's stock gray bubble. Restyling them to the design system looks
like the point of the migration rather than collateral damage. If we accept that,
the marker goes, the override goes global, and the wrapper's last justification
goes with it.
You may know something about Sketch Lab's intent here that I can't see from the
code — if those tooltips are deliberately stock, say so and the marker stays
(and then I'd keep the wrapper too, since something has to set it).
Two things this PR surfaces that belong in the theme regardless
Both are follow-ups, not blockers for this one:
CdoThemesets nodirection, so theuseTheme().direction === 'rtl'
guard inTooltip.tsxis dead code today and the mirroring is hand-rolled off
<html dir>. Settingdirectionwhere the theme is built gets the mirroring
for free and activates MUI's ownownerState.isRtlarrow-position styles,
which are currently inert for every popper in the app.- The portal/
data-themeproblem isn't tooltip-specific — Sketch Lab
already re-appliesdata-themeby hand forPopover
(ToolbarDropdownRow.tsx:168). Either the call site passes
slotProps={{tooltip: {'data-theme': theme}}}, or the bubble uses-fixed
tokens so it doesn't care about the surrounding theme at all and the prop
disappears.
One cost of my own ask, to be fair to it
Dropping the wrapper loses keyboardOnly as a discoverable affordance —
disableHoverListener disableTouchListener at a call site doesn't communicate
"this hint exists for keyboard users only," and that intent is worth keeping.
Exporting a named props object (export const keyboardOnlyTooltipProps = {…})
holds onto the name and the docs while staying inside the pattern, much like
button/ exports its buttonPropsToMuiCore maps rather than components.
|
Follow-up to my review above — I turned the PoC into something you can look at One warning that cost me twenty minutes: run The principle I'd like us to settle onBeyond the wrapper question, and the more important half of this review: The payoff is that retiring
Worth writing into the README while you're in here: the legacy tooltip defaults |
|
Thanks so much @moshebaricdo and @levadadenys for the reviews. Here's an overview of what I changed with Claude's help: Went theme-only (per Denys) Dropped Tooltip.tsx entirely — call sites import Tooltip from @mui/material directly now, same as Button/Breadcrumbs. Relied on the POC to get the right feel. The override is global now (no more data-cdo-tooltip marker), so the Sketch Lab tooltips pick up the DS look for free. Verified that every trigger already has its own aria-label, so flipping describeChild on doesn't strip any names. Matched the spec (per Moshe) One fixed size (small / body3), size options dropped. Thanks again both! |
moshebaricdo
left a comment
moshebaricdo
left a comment
There was a problem hiding this comment.
Ty for the updates!! Pulled locally and it looks great. I did some stress testing and caught some small differences from the spec. Opened a draft pr and pulled this branch to make some of those last-mile visual tweaks (w/ AI obvi lol).
I also noticed that in all of the stories I can hover over the tooltip itself after hovering over the trigger element and it is prevented from disappearing which is prob behavior we would want as on option but not as the default, didn't mess with tweaking that behavior in my draft pr.
Eek thank you! Want to merge that into this or as a follow up? And as for the staying on hover that's an accessibility requirement for all tooltips so I'd actually love to keep that! |
levadadenys
left a comment
levadadenys
left a comment
There was a problem hiding this comment.
This looks great — the theme-only shape came out cleaner than what I sketched, and two of my worries resolved themselves: CADS dropping size options means there's no size prop to leak at all, and going global answers the Sketch Lab question rather than working around it. Verified on your branch: yarn typecheck clean, 371 tests pass.
Inline comments are all leftovers from the wrapper era — comments and code that described a design that's now gone. Two substantive items:
textAlign: 'left' wants to be 'start'. The old SCSS used center, which is direction-agnostic, so this is newly introduced: in an RTL locale a wrapping tooltip now sits flush left. start keeps the CADS left-alignment in LTR and does the right thing in RTL. Worth it here specifically because the README documents RTL as handled.
The README's data-theme example doesn't compile, which is why the story needs as never. MUI names the fix in the error it raises — TooltipTooltipSlotPropsOverrides is its extension point for exactly this. Details in the inline comment; I confirmed the augmentation makes the README snippet typecheck verbatim while still rejecting a bad value.
One heads-up rather than a request: going global changes how Sketch Lab's five tooltips look (MUI gray → DS dark with a tail). Nothing in the e2e specs or cucumber features asserts on them, so CI won't flag it either way — which is why it's probably worth @moshebaricdo glancing at a Sketch Lab toolbar before this merges.
I've closed my PoC draft (#74612) since this supersedes it.
| // Tooltip has no `size` here on purpose: MUI's Tooltip copies unknown props | ||
| // onto the trigger, so its size travels as a `data-size` attribute instead. | ||
| // See src/themes/code.org/styleOverrides/tooltip.ts. |
There was a problem hiding this comment.
This comment now describes code that doesn't exist. There's no size prop anywhere (CADS made the tooltip one fixed size) and no data-size attribute — styleOverrides/tooltip.ts, which it points at, never mentions size. It's preserving the reasoning of a design that got dropped, with no code under it.
Either delete it, or reduce it to the fact that survives: the CADS tooltip is one fixed size, so there's nothing to augment.
Though if you take the data-theme suggestion below, this is exactly where it goes — the file does need one augmentation after all, just not for size:
declare module '@mui/material/Tooltip' {
interface TooltipTooltipSlotPropsOverrides {
'data-theme'?: Theme; // from '@/common/contexts'
}
}|
|
||
| const renderTooltip = ( | ||
| props: Record<string, unknown> = {}, | ||
| child?: React.ReactElement, |
There was a problem hiding this comment.
child is never passed a value — all ten renderTooltip(...) call sites leave it out, and its only effect is forcing the placeholder in renderTooltip({placement: 'bottom-start'}, undefined, rtlTheme) at line 134.
Dropping it makes that call read renderTooltip({placement: 'bottom-start'}, rtlTheme).
| // A plain MUI tooltip with no special props still gets the design system look. | ||
| it('styles a tooltip that sets no design-system props at all', async () => { | ||
| render( | ||
| <ThemeProvider theme={CdoTheme}> | ||
| <MuiTooltip title="Duplicate" placement="top"> | ||
| <button type="button">trigger</button> | ||
| </MuiTooltip> | ||
| </ThemeProvider>, | ||
| ); | ||
| const bubble = await openAndGetBubble(); | ||
| expect(getComputedStyle(bubble).backgroundColor).toBe(BACKGROUND); | ||
| expect(document.querySelector('.MuiTooltip-arrow')).not.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
I think this one can go: every test in the file is now a bare MUI tooltip with no design-system props — renderTooltip() is this case — so the distinction the title and comment draw no longer exists. The two assertions duplicate "uses our CADS color, shape, and shadow tokens" and "shows a tail by default".
It earned its place when data-cdo-tooltip existed and a bare MUI tooltip was genuinely a different path. Now it also re-inlines render(<ThemeProvider>…) rather than using the helper right above it.
| maxWidth: '16rem', | ||
| width: 'max-content', | ||
| padding: '0.25rem 0.75rem', | ||
| textAlign: 'left', |
There was a problem hiding this comment.
'start' rather than 'left' — see the top-level note. left is physical, so it doesn't follow text direction, and the SCSS this replaces used center and so never had the problem.
| width: '0.875rem', | ||
| fontSize: '0.875rem', | ||
| lineHeight: 1, | ||
| color: FOREGROUND, |
There was a problem hiding this comment.
Possibly redundant: the bubble already sets color: FOREGROUND at line 24 and an <i> inherits it. I couldn't find the FontAwesome CSS to confirm nothing sets a color on i — if it doesn't, this line can go. Flagging as unverified rather than as a request.
| surrounding `data-theme` subtree. Pass it through `slotProps` when needed: | ||
|
|
||
| ```tsx | ||
| <Tooltip title="…" slotProps={{tooltip: {'data-theme': 'Dark'}}}> |
There was a problem hiding this comment.
This example doesn't typecheck as written:
error TS2353: ''data-theme'' does not exist in type
'SlotProps<"div", TooltipTooltipSlotPropsOverrides, TooltipOwnerState>'
which is why Tooltip.story.tsx needs as never. That cast is worth getting rid of on its own merits — as never switches off checking for the whole object, so a typo'd key or a bogus theme value would pass silently.
MUI names the remedy in the error: TooltipTooltipSlotPropsOverrides is its extension point for extra props on the tooltip slot. Adding the augmentation (see my comment on muiAugmentation.ts) makes this snippet compile exactly as documented, and a wrong value still errors — I checked both, including a @ts-expect-error probe on 'data-theme': 'Nope'.
There was a problem hiding this comment.
Looks great, thank you very much!
Left a final me+claude review above with some minor/redundant code/comments worth polishing.
Approving the PR in advance
Leaving this note here just in case: Next overall migration steps/prs would be:
- migrating WithTooltip and LegacyTooltip usages to use Mui Tooltip (I used to do this via creating a codemod and then running the codemod through the codebase, then verifying each usage in code manually. But also worth trying just running a claude opus/fable in couple sessions to locate and migrate each usage)
- deprecating
LegacyTooltipandWithTooltipcomponents
Yes please to merging in, and thank you for the context on the proper behavior for tooltips! |
This PR was borne from my need to have a keyboard only tooltip. That is, I want to be able to surface keyboard shortcuts when users tab into a button whose shortcuts are not available anywhere else. We lost points on our VPAT because even though some actions were keyboard operable, there was no way for a user to know how. But instead of just creating a new tooltip for this purpose, it is the right time to pull in MUI which can then replace our existing tooltip component and provide this new functionality too.
Slack context here: https://codeai.slack.com/archives/C0532LCK125/p1784676197629769
I am not familiar with MUI, so relied heavily on Claude to do this work. That said, between copilot and claude I ended up with a lot of iterations that look good in the resulting storybook, so hopefully it's close. I also had Claude copmare with the design spec (thanks Moshe!).
Claude:
Adds a new
Tooltipto@code-dot-org/component-library, built on MUI's, along with the theme styles that make it look like ours. This is the first step of the tooltip migration. Nothing switches over to it yet, and no tooltip anywhere in the app changes.It takes all of MUI's tooltip props, plus a few of ours:
size,iconNamefor a leading icon,hasCaretto turn the tail off,data-theme, andkeyboardOnly. That last one shows the tooltip only when someone tabs to the trigger, never on hover — useful for a hint that a mouse user doesn't need but a keyboard user has no other way to get.Two of MUI's defaults are flipped: tooltips get a tail, and the text describes the trigger rather than naming it. So each trigger needs its own accessible name.
How this differs from past migrations
Button (#70158) and Breadcrumbs (#70161) added theme styles and nothing else, and callers just import from
@mui/material. This one departs in four ways.keyboardOnlyandiconNamechange how the tooltip behaves, and theme styles can only change how it looks.data-sizeattribute, not a prop. MUI's tooltip hands any prop it doesn't recognize to the element you wrap. Asizeprop therefore lands on the trigger too, and anIconButtontrigger has its ownsizethat ours would overwrite, shrinking the button down around its icon. Breadcrumbs can use a prop because it doesn't wrap anything. There's a test for this.LegacyTooltip. The new one needed theTooltipname. Nothing imports the old one today, andTooltipPropsstill refers to it.Everything else is the usual shape: styles in
styleOverrides/tooltip.ts, text sizes taken from the theme instead of written out again, and aMIGRATION_STATUS.mdrow set to In Progress.Right-to-left
Placements like
bottom-startopen from the other side in a right-to-left language. Plainleftandrightdon't move, which matches MUI: a physical side stays where you put it. The old tooltip did flip itsonLeft/onRight, so if you move a caller across and the side was meant to follow the text, switch it to-start/-end.Nothing about the tooltip's own appearance needs mirroring. Its padding is the same on both sides, and the icon sits beside the text using flexbox, which follows the text direction already.
MUI can do this flipping for us, but only if the theme says which direction the page runs in. Setting that would flip every MUI pop-up in the app — menus, selects, autocompletes, popovers, drawers — and none of those have been checked in a right-to-left language. That's worth doing, in its own PR. Until then the tooltip reads the direction off the page itself, and steps aside if the theme is ever given one, so the two can't undo each other.
What doesn't change
WithTooltipand its ~35 callers, and every place already using MUI's tooltip directly. Three tests render a plain MUI tooltip and check it still has MUI's own tail, naming and colors, so nobody widens these styles by accident later.Testing
48 tooltip tests, 404 across the package, 302 in Sketch Lab. Type checking and linting pass in both
component-libraryandapps.The
WithTooltiptests that used to live inTooltip.test.tsxmoved intoWithTooltip.test.tsxrather than being dropped.Storybook now has
Tooltip/TooltipandTooltip/WithTooltipside by side. Renaming the old group changed its story ids, so Applitools will see those as brand new stories that need baselines approved, rather than as differences.Follow-ups
CdoThemewhich direction the page runs in, so every MUI pop-up flips, not just this one.WithTooltip.hasCaretis a third name for the same thing (MUI calls itarrow, the old one called ithideTail). Probably worth dropping.End of Claude!
Proof of the new keyboard only in storybook:
Screen.Recording.2026-08-12.at.2.15.09.PM.mov
Links
Testing story
Deployment notes
Privacy and security