Fix tooltip accent line and colors#18
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModified the RadarChart component's tooltip styling by replacing CSS variable references with hard-coded color values for the tooltip background, text labels, and decoration polygons. Added an SVG clipPath element to constrain the tooltip indicator bar within tooltip boundaries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/RadarChart.jsx`:
- Line 413: The component RadarChart.jsx currently hardcodes tooltip colors
(e.g., fill="#1e293b" and the inconsistent "#475569" used for the "/ 4" suffix)
instead of using CSS custom properties; add dedicated tooltip variables to
src/theme.js (for example --tooltip-bg, --tooltip-text, --tooltip-text-muted)
and update RadarChart.jsx to replace all hardcoded color literals (the fill and
any color props rendering the muted suffix and other tooltip text) with
references to those new CSS variables so the tooltip remains consistently dark
while honoring the theme system.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6294f8ba-06fb-4eef-872e-50a054e8095b
📒 Files selected for processing (1)
src/components/RadarChart.jsx
| height={tooltipH} | ||
| rx={10} | ||
| fill="var(--bg-card)" | ||
| fill="#1e293b" |
There was a problem hiding this comment.
Hardcoded colors bypass theme system and contain an inconsistency.
While the PR intentionally uses fixed dark colors for an "always-dark" tooltip, this approach:
- Violates the CSS custom properties guideline for theme management.
- Contains an inconsistent color: Line 424 uses
#475569, which is the light mode--text-mutedvalue. The dark mode equivalent is#cbd5e1. This will make the "/ 4" suffix appear darker than expected in the dark-styled tooltip.
If always-dark tooltips are a deliberate design pattern, consider defining dedicated CSS variables (e.g., --tooltip-bg, --tooltip-text-muted) in theme.js that remain constant across themes. This preserves the design intent while maintaining the theme system's extensibility.
Suggested fix for color consistency
- <tspan fill="#475569" fontSize="11" fontWeight="400">
+ <tspan fill="#cbd5e1" fontSize="11" fontWeight="400">Or better, add dedicated tooltip variables to theme.js that don't change between themes:
// In theme.js, add to both palettes:
"--tooltip-bg": "#1e293b",
"--tooltip-text": "#94a3b8",
"--tooltip-text-muted": "#cbd5e1",Then reference these in the component to maintain theme system compliance.
As per coding guidelines: "Implement theme management via CSS custom properties defined in src/theme.js".
Also applies to: 419-419, 424-424, 431-431, 438-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RadarChart.jsx` at line 413, The component RadarChart.jsx
currently hardcodes tooltip colors (e.g., fill="#1e293b" and the inconsistent
"#475569" used for the "/ 4" suffix) instead of using CSS custom properties; add
dedicated tooltip variables to src/theme.js (for example --tooltip-bg,
--tooltip-text, --tooltip-text-muted) and update RadarChart.jsx to replace all
hardcoded color literals (the fill and any color props rendering the muted
suffix and other tooltip text) with references to those new CSS variables so the
tooltip remains consistently dark while honoring the theme system.
- Remove left accent bar from tooltip (was oval due to rx=4 on width=4, not present on the right side — asymmetric and visually broken) - Use fixed dark colors for tooltip (always dark like OS tooltips) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4097384 to
5ba5e30
Compare
Summary
rx={4}on a 4px-wide rect, causing oval shape that didn't cover the stroke)clipPathto clip accent bar to main rect's rounded cornersvar(--bg-card)was white in light mode)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Style
Refactor