fix(Tooltip): align styles with design guidelines (DS-5340) - #479
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Tooltip component now uses compact typography, has no enforced minimum dimensions, and uses the contrast foreground color for warnings. Tooltip Storybook metadata no longer includes status and date tags. ChangesTooltip presentation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The Tooltip styling updates are bounded to the intended presentation changes, with no actionable merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Stylelint (17.14.0)packages/components/src/components/Tooltip/Tooltip.module.cssConfigurationError: Could not find "stylelint-config-css-modules". Do you need to install the package or use the "configBasedir" option? Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Visit the preview URL for this PR (updated for commit 592b232): https://react-koobiq-next--prs-479-bm9xwt4k.web.app (expires Wed, 16 Sep 2026 10:25:58 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: fc29847d4a9e5cb1adf458c76a9b681c76e2eeff |
There was a problem hiding this comment.
🟢 Approval recommended
The two-line visual tweak uses a verified typography utility and a consistent min-height value, with no API, test, or documentation regressions.
Pull request overview
This PR makes the Tooltip component more compact to align with updated design guidelines (DS-5340). It reduces the tooltip's typography scale and minimum height so the overlay takes up less vertical space.
Changes:
- Switched the tooltip text from the
text-normaltypography utility to the smallertext-compactutility. - Reduced the tooltip's
min-block-sizefrom36pxto32pxto match the compact typography.
File summaries
| File | Description |
|---|---|
packages/components/src/components/Tooltip/Tooltip.tsx |
Applies the text-compact typography utility class instead of text-normal. |
packages/components/src/components/Tooltip/Tooltip.module.css |
Lowers min-block-size from 36px to 32px for the more compact layout. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| s.base, | ||
| s[variant], | ||
| utilClasses.typography['text-normal'] | ||
| utilClasses.typography['text-compact'] |
There was a problem hiding this comment.
offset={0} is silently turned into 4. (Actual line: 76 — outside this hunk, so anchoring here.)
const offset = showArrow ? offsetProp : offsetProp || 4;|| treats 0 as absent, so the no-arrow branch (the default, since hideArrow defaults to true) rewrites a deliberate offset={0} into 4. A consumer asking for a tooltip flush against its trigger silently gets a 4px gap, with no way to express "zero".
Reproducible from the Offsets story: type 0 into the offset input and the tooltip stays 4px off the trigger.
const offset = showArrow ? offsetProp : (offsetProp ?? 4);?? only substitutes for null/undefined, which is what "no offset passed" actually means here.
There was a problem hiding this comment.
Confirmed, and the JSDoc says @default 0 as well. Pre-existing, so it goes to a separate fix: ?? plus a JSDoc update (DS-5558).
| s.base, | ||
| s[variant], | ||
| utilClasses.typography['text-normal'] | ||
| utilClasses.typography['text-compact'] |
There was a problem hiding this comment.
Overflow-reveal tooltips now render the revealed text smaller than the text they reveal.
Two in-repo components use Tooltip purely as an "show me the part that got truncated" affordance, and both render the source text at 14px:
FileUpload/components/FileUploadItem/FileUploadItem.tsx—FileUploadItemNamewraps the ellipsised filename in aTooltipgated onisOverflowing, andFileUploadList.tsx:26renders the list atutilClasses.typography['text-normal'](14px).Navbar/components/NavbarItem.tsx— the same{children}is rendered in theLinkattypography['text-normal-medium'](14px) and passed to theTooltipto be revealed when the navbar is collapsed.
After this change the tooltip shows that same string at text-compact (12px / 16px line-height), so the "full" text is visibly smaller than the truncated original it is standing in for. Since the design guideline is about tooltips-as-hints, it's worth confirming with design that the truncation-reveal case is meant to shrink too — if not, these two call sites need a way to opt back into text-normal (see the separate note about there being no override point).
There was a problem hiding this comment.
Intended: the design token --kbq-tooltip-font-default-* is text-compact, and the Angular tooltip uses text-compact everywhere, including the navbar. So the truncation-reveal case shrinks too.
| s.base, | ||
| s[variant], | ||
| utilClasses.typography['text-normal'] | ||
| utilClasses.typography['text-compact'] |
There was a problem hiding this comment.
The status:updated badge for this change will not show — the date: tag is stale. (Actual location: Tooltip.stories.tsx:26, a file this PR doesn't touch, so anchoring here.)
tags: ['status:updated', 'date:2026-05-15'],.storybook/manager.tsx:45 gives status:updated a 30-day TTL, so isStatusBadgeExpired considers this badge expired since 2026-06-14. AGENTS.md: "For new/updated, include a date:YYYY-MM-DD tag — Storybook uses it to expire temporary badges." This PR is a user-visible restyle, which is exactly what the badge is for, so the date should be bumped to this PR's date and status:updated kept.
There was a problem hiding this comment.
Removed the stale status:updated / date: tags instead of bumping the date.
| s.base, | ||
| s[variant], | ||
| utilClasses.typography['text-normal'] | ||
| utilClasses.typography['text-compact'] |
There was a problem hiding this comment.
There's no supported way for a consumer to keep the old size.
The typography class is baked in here and then merged via React Aria's mergeProps, which concatenates className rather than replacing it (mergeProps.mjs:34: key === 'className' → clsx(a, b)). So a consumer's own class lands alongside .text-compact, not instead of it, and whichever rule wins comes down to emission order in dist/style.css — in practice they need !important or a specificity hack.
AGENTS.md's documented escape hatch is a public --kbq-<component>-* variable, but Tooltip.module.css declares only private ones (--tooltip-bg-color, --tooltip-color, --tooltip-arrow-size) and wires none of them to a --kbq-tooltip-* override, so the component currently exposes no override points at all.
Tooltip is <Status variant="experimental" /> and the lifecycle doc allows breaking changes in minor releases, so this isn't a blocker — but since this PR is the change that makes someone want the old value, it's the natural moment to add a --kbq-tooltip-* hook (at minimum for font-size/line-height, ideally for the three existing private vars too).
There was a problem hiding this comment.
Agreed it's worth having, but it's new public API, so it goes to a separate task (DS-5559).
- use dark text on the warning variant, as in the Angular reference - drop min-inline-size and min-block-size - remove the stale status:updated badge from stories Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
KamilEmeleev
changed the title
Summary by CodeRabbit