Repository navigation
feat: Line chart legend - #706
Conversation
📝 WalkthroughWalkthroughAdds a configurable ChangesLine Chart Legend Feature and Supporting Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0664ffb0f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsx`:
- Around line 57-65: The "Show Legend" label is not associated with the Switch,
making the control inaccessible; update the Switch in LineChartSettings so it
has an accessible name tied to the label (e.g., give the Switch an id like
"show-legend-switch" and set the label's htmlFor to that id, or add a clear
aria-label/aria-labelledby) and ensure the onCheckedChange still calls
onChangeConfig('showLegend', showLegend) and the component still reads initial
state from config.settings.showLegend.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7f630a7-e57b-47e9-9b89-1a8ffe2dc265
📒 Files selected for processing (7)
packages/mosaic/__tests__/chart-types/line-chart-spec.test.tspackages/mosaic/src/VgPlotChart.tsxpackages/mosaic/src/charts/chart-types/histogram/spec.tspackages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsxpackages/mosaic/src/charts/chart-types/line-chart/schema.tspackages/mosaic/src/charts/chart-types/line-chart/spec.tspackages/mosaic/src/useVgPlotChartRender.ts
|
I think the Show legend switch is a bit too large, I'd make it smaller, also I would place it on the left of the label and maybe change the background to not use the primary color? It feels a bit too visually intense. e.g. here it doesn't look that large: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88bfe4b4a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/switch.tsx (1)
8-25:⚠️ Potential issue | 🟠 MajorFix SwitchRoot default to use the styled Thumb
SwitchRootfalls back to{children ?? <SwitchPrimitives.Thumb />}, so a self-closing<Switch />renders Radix’s unstyled thumb (noSwitchThumbclasses).packages/kepler/src/components/CustomInteractionManager.tsxuses<Switch ... />with no children, so it will miss the styled thumb; by contrastpackages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsxcorrectly renders<Switch.Thumb />. Use<SwitchThumb />as the fallback (or document that callers must provide<Switch.Thumb />).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/switch.tsx` around lines 8 - 25, SwitchRoot currently falls back to Radix’s unstyled SwitchPrimitives.Thumb when no children are provided, causing unstyled thumbs for plain <Switch /> usages; update the fallback inside the SwitchRoot forwardRef (where SwitchPrimitives.Root is rendered) to use the styled SwitchThumb component instead of SwitchPrimitives.Thumb so a self-closing <Switch /> renders the correct styled thumb. Ensure the ref/props behavior remains unchanged and keep SwitchRoot.displayName as-is.
🧹 Nitpick comments (1)
packages/ui/src/components/switch.tsx (1)
42-46: ⚡ Quick winDocument the compound component API for public consumers.
The
Switch.Thumbcompound component API is exported frompackages/uibut lacks JSDoc documentation. As per coding guidelines, public exports should be documented. Consider adding JSDoc that explains:
- How to use
<Switch />with the default thumb versus<Switch><Switch.Thumb /></Switch>for custom styling- The props accepted by both
SwitchandSwitch.Thumb- Example usage patterns (basic toggle vs custom content like icons)
📝 Example documentation structure
+/** + * A flexible switch component built on Radix UI primitives. + * + * `@example` + * // Basic switch with default thumb + * <Switch checked={isEnabled} onCheckedChange={setIsEnabled} /> + * + * `@example` + * // Switch with custom styled thumb + * <Switch checked={isEnabled} onCheckedChange={setIsEnabled}> + * <Switch.Thumb className="custom-styles" /> + * </Switch> + * + * `@example` + * // Switch with custom content + * <Switch checked={isDark} onCheckedChange={setIsDark}> + * <Switch.Thumb> + * <SunIcon /> + * </Switch.Thumb> + * </Switch> + */ const Switch = Object.assign(SwitchRoot, { + /** Styled thumb component for use within Switch */ Thumb: SwitchThumb, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/switch.tsx` around lines 42 - 46, Add JSDoc for the public compound component export where Switch is composed from SwitchRoot and SwitchThumb (the Object.assign block that creates Switch from SwitchRoot and Thumb: SwitchThumb). Document usage patterns: default thumb via <Switch /> and custom thumb via <Switch><Switch.Thumb /></Switch>, list accepted props for Switch (e.g., checked, onChange, disabled, className) and for Switch.Thumb (e.g., className, children) and include two short examples (basic toggle and custom content like an icon). Place the comment immediately above the const Switch = Object.assign(...) export so it appears in the generated docs for both Switch and Switch.Thumb.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ui/src/components/switch.tsx`:
- Around line 8-25: SwitchRoot currently falls back to Radix’s unstyled
SwitchPrimitives.Thumb when no children are provided, causing unstyled thumbs
for plain <Switch /> usages; update the fallback inside the SwitchRoot
forwardRef (where SwitchPrimitives.Root is rendered) to use the styled
SwitchThumb component instead of SwitchPrimitives.Thumb so a self-closing
<Switch /> renders the correct styled thumb. Ensure the ref/props behavior
remains unchanged and keep SwitchRoot.displayName as-is.
---
Nitpick comments:
In `@packages/ui/src/components/switch.tsx`:
- Around line 42-46: Add JSDoc for the public compound component export where
Switch is composed from SwitchRoot and SwitchThumb (the Object.assign block that
creates Switch from SwitchRoot and Thumb: SwitchThumb). Document usage patterns:
default thumb via <Switch /> and custom thumb via <Switch><Switch.Thumb
/></Switch>, list accepted props for Switch (e.g., checked, onChange, disabled,
className) and for Switch.Thumb (e.g., className, children) and include two
short examples (basic toggle and custom content like an icon). Place the comment
immediately above the const Switch = Object.assign(...) export so it appears in
the generated docs for both Switch and Switch.Thumb.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d1451e7-82b0-4a24-9421-1c2a5f9eaa85
📒 Files selected for processing (3)
packages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsxpackages/ui/src/components/switch.tsxpackages/ui/src/components/theme-switch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mosaic/src/charts/chart-types/line-chart/LineChartSettings.tsx
Summary by CodeRabbit
New Features
Improvements
Tests