-
Notifications
You must be signed in to change notification settings - Fork 25
feat: shadcn piechart and group as "Others" #2798
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughPieChartPanel component refactored to introduce memoized data processing, enhanced color resolution logic, and bucketing of excess slices into an "others" category. The chart rendering migrated from ResponsiveContainer to ChartContainer with updated tooltip and legend components. Function signature of generatePieChartData extended with maxSlices parameter. Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
❌ Deploy Preview for flanksource-demo-stable failed. Why did it fail? →
|
❌ Deploy Preview for clerk-saas-ui failed. Why did it fail? →
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/audit-report/components/View/panels/PieChartPanel.tsx (2)
54-73: Edge case:maxSlices = 1produces 2 slices.When
maxSlicesis 1, the logic keeps 1 slice and adds "others", resulting in 2 total slices. IfmaxSlicesis intended to be the exact maximum count including "others", consider adjusting:- const keep = sorted.slice(0, Math.max(maxSlices - 1, 1)); + const keep = sorted.slice(0, maxSlices - 1);However, if the intent is "at least 1 real slice + others", the current behavior is fine. Worth clarifying the expected behavior for edge cases.
89-91: Consider passingmaxSlicesas a configurable prop.The
maxSlicesparameter defaults to 6 ingeneratePieChartData, but it's not exposed as a prop onPieChartPanel. If this should be configurable per chart instance (e.g., viasummary.piechart?.maxSlices), consider wiring it through.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/audit-report/components/View/panels/PieChartPanel.tsx
🔇 Additional comments (3)
src/pages/audit-report/components/View/panels/PieChartPanel.tsx (3)
1-12: LGTM!The imports are well-organized and align with the refactored component's use of shadcn chart components.
26-52: LGTM!The data transformation logic is solid:
- Safe numeric parsing with
Number.isFinitecheck- Fallback to "Unknown" for missing labels
- Well-structured color priority: custom → semantic → palette
107-130: LGTM!Good migration to ChartContainer with proper tooltip and legend integration. The conditional stroke removal for single-slice pies is a nice UX touch.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.