Add ProportionChart component for data visualization#30
Conversation
- Add ProportionChart component (migrated from RibbonGraph in gp-dixie) - Displays data as horizontal segmented bar showing proportions - Features: interactive tooltip, optional legend, dark mode support - Includes input validation for negative values and missing color configs - Pre-calculates percentages once for DRY principle - Enhanced accessibility with ARIA labels and semantic HTML - Comprehensive test suite with 20+ test cases - Full demo with 6 sections showing different use cases - Integrated into playground after StatGrid with BarChart4 icon Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Preview deployed! 🚀View the playground preview at: https://thegpvc.github.io/gp-ui/pr-30/ Last updated: 2026-01-21T00:07:40.611Z |
Code Review: ProportionChart ComponentGreat work on this PR! The ProportionChart component is well-implemented with excellent test coverage and comprehensive documentation. Here's my detailed review: ✅ StrengthsCode Quality
Testing
Accessibility
Documentation
🔍 Issues & Suggestions1. Potential Division by Zero Edge Case (Minor)Location: While you filter out zero-value segments (line 130), if the total is calculated from an empty array or all zeros, division by zero could occur at line 147 before the total === 0 check at line 156. Current flow: const validSegments = data.filter((d) => d.value > 0) // line 130
// ... validation ...
const total = validSegments.reduce((sum, d) => sum + d.value, 0) // line 143
const segments = validSegments.map((d) => {
const percentage = (d.value / total) * 100 // line 147 - division happens here
// ...
})
if (total === 0) { ... } // line 156 - check happens afterRecommendation: Move the total === 0 check before the percentage calculations: const total = validSegments.reduce((sum, d) => sum + d.value, 0)
// Check for empty state before calculating percentages
if (total === 0) {
return (/* empty state JSX */)
}
// Now safe to calculate percentages
const segments = validSegments.map((d) => {
const percentage = (d.value / total) * 100
// ...
})This is a defensive programming practice, even though the current code works because 2. Inconsistent aria-label on Segments (Minor)Location: Individual segment divs have aria-labels, but they're decorative parts of a larger role="img" element. The outer container already has comprehensive aria-label. Current: <div
key={seg.key}
className={cn(seg.bg, 'transition-opacity hover:opacity-80')}
style={{ width: `${seg.percentage}%` }}
aria-label={`${seg.label}: ${formatValue(seg.value)} (${seg.percentage.toFixed(0)}%)`}
/>Recommendation: Consider removing aria-label from individual segments or marking them as 3. Tooltip Accessibility (Minor)Location: The tooltip contains a Recommendation: Either:
4. Magic Number in Tests (Minor)Location: The test calculates Recommendation: Make the test more explicit: it('calculates percentages correctly', () => {
const total = mockData.reduce((sum, d) => sum + d.value, 0) // 100
const completedValue = mockData[0].value // 60
const expectedPercentage = (completedValue / total) * 100 // 60%
const { container } = render(...)
const completedSegment = container.querySelector('.bg-green-500')
expect(completedSegment).toHaveStyle({ width: `${expectedPercentage}%` })
})5. Theme CSS Could Be More Specific (Very Minor)Location: The Current approach is fine, but worth considering if future styling needs justify the base class. 🎯 Design System Alignment✅ Follows CLAUDE.md conventions:
✅ Follows DESIGN_GUIDE.md:
🔒 Security & Performance✅ No security concerns:
✅ Performance is good:
📊 SummaryThis is a high-quality PR that's ready to merge with only minor suggestions. The component is well-tested, accessible, and follows all established patterns in the codebase. Recommendation: ✅ Approve with minor suggestions The suggestions above are nitpicks that would make the code slightly more robust, but they don't block merging. The current implementation is solid and production-ready. Nice work! 🎉 |
- Fix division by zero: Move total === 0 check before percentage calculations - Improve accessibility: Remove redundant aria-labels from segments, mark as aria-hidden - Fix tooltip ARIA: Remove improper role="row" and role="cell" attributes - Improve test clarity: Make percentage calculation explicit in test Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Code Review: ProportionChart ComponentOverall, this is an excellent addition to the design system! The implementation is well-crafted, thoroughly tested, and follows the repository's conventions closely. The code quality is high with comprehensive documentation and accessibility features. ✅ StrengthsCode Quality
Accessibility
Testing
Documentation
Design System Alignment
🔍 Areas for Improvement1. Minor: Tooltip Content Dark Mode (Line 1038-1056)The tooltip uses a grid layout with borders, but the border color might not have optimal contrast in all scenarios: // Line 1053: Consider adding dark mode variant
<div className="mt-1 pt-1 border-t border-gray-600 dark:border-gray-500 flex justify-between">The current 2. Minor: Empty State Typography ConsistencyIn the empty state (line 999), the text uses <span className="text-xs text-gray-500 dark:text-gray-400">No data</span>
// Consider: text-sm for better readability3. Test Coverage: Tooltip InteractionWhile the tests verify tooltip markup exists (lines 788-812), they don't test the actual tooltip interaction (hover behavior). Consider adding a test that simulates hover and verifies tooltip content appears. Example: import { userEvent } from '@testing-library/user-event'
it('shows tooltip on hover with formatted values', async () => {
const user = userEvent.setup()
const { container } = render(<ProportionChart ... />)
const bar = container.querySelector('.cursor-pointer')
await user.hover(bar!)
// Verify tooltip content appears
await waitFor(() => {
expect(screen.getByText('Total')).toBeVisible()
})
})4. Documentation: Missing Usage PatternThe demo and docs don't show an example of handling dynamic data updates. Consider adding a note or example about what happens when data changes (does the chart smoothly update? Should consumers use keys?). 5. Performance: Memoization OpportunityThe const segments = useMemo(() =>
validSegments.map((d) => {
const percentage = (d.value / total) * 100
return { ...d, ...colors[d.key], percentage }
}),
[validSegments, total, colors]
)However, this is only needed if performance becomes an issue in practice. 6. Minor: Legend Grid BreakpointsLine 1064: // Current: grid-cols-2 sm:grid-cols-5
// Consider: grid-cols-2 sm:grid-cols-3 lg:grid-cols-5This provides a more graceful transition on medium-sized screens. 🛡️ Security & Performance
📋 Best Practices Checklist
🎯 RecommendationsPriority: Merge as-is ✅ This PR is ready to merge. The suggested improvements above are optional enhancements and not blockers. The component is production-ready with excellent code quality, comprehensive tests, and strong accessibility support. Optional Follow-ups (can be addressed in future PRs if desired):
Great work! This component will be a valuable addition to the design system. The migration from RibbonGraph shows thoughtful redesign and significant improvements. 🎉 |
| @@ -0,0 +1,282 @@ | |||
| import { ProportionChart, type ColorConfig } from '@gp/ui' | |||
|
|
|||
| export function ProportionChartDemo() { | |||
There was a problem hiding this comment.
Naming thought: <meter> exists, so could potentially align with that naming.
Replace generic Tailwind colors with theme semantic colors: - Status colors: emerald (success), navy (info), amber (warning), rose (error) - Time/resource colors: emerald, navy where applicable - Keep purple and cyan for variety (no theme equivalent) - Add dark mode variants for all theme colors All colors now follow DESIGN_GUIDE.md semantic color palette. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
PR Review: ProportionChart ComponentSummaryThis is a well-implemented data visualization component with excellent test coverage, comprehensive validation, and strong accessibility support. The code quality is high and follows the repository conventions outlined in CLAUDE.md. ✅ StrengthsCode Quality
Test Coverage
Accessibility
Documentation
Design System Compliance
🔍 Suggestions & Minor Issues1. Color Class Validation (Medium Priority)Issue: The component accepts arbitrary Tailwind classes in Current code (ProportionChart.tsx:849): export interface ColorConfig {
bg: string // Could be any string, not necessarily a valid bg class
label: string
}Potential problem: // This would be accepted but wouldn't render correctly:
colors={{ foo: { bg: 'text-red-500', label: 'Foo' } }}Suggestion: Add a runtime warning in development mode or enhance the TypeScript type: // Option 1: Type-level guidance (TypeScript only, no runtime)
export interface ColorConfig {
/** Tailwind background color class (e.g., 'bg-blue-500', 'bg-navy-500 dark:bg-navy-300') */
bg: string & { __brand?: 'TailwindBgClass' }
label: string
}
// Option 2: Runtime validation in dev mode
if (process.env.NODE_ENV === 'development') {
Object.entries(colors).forEach(([key, config]) => {
if (!config.bg.includes('bg-')) {
console.warn(
`ProportionChart: Color config for "${key}" may be invalid. ` +
`Expected a background class (e.g., 'bg-blue-500'), got: "${config.bg}"`
)
}
})
}2. Legend Responsive Breakpoint (Low Priority)Issue: The legend uses Current code (ProportionChart.tsx:1064): className="mt-3 grid grid-cols-2 sm:grid-cols-5 gap-3"Observation: Jump from 2 columns to 5 columns is large. With 3-4 items, this works well. With 7+ items, medium screens might look awkward. Suggestion: Consider a more gradual progression: className="mt-3 grid grid-cols-2 sm:grid-cols-3 lg:grid-cols-5 gap-3"3. Tooltip Percentage Precision (Very Low Priority)Issue: Percentage is rounded to 0 decimals which might hide small segments. Current code (ProportionChart.tsx:1048): {seg.percentage.toFixed(0)}%Example: A segment that's 0.8% would show as "1%" while 0.4% shows as "0%". Suggestion: Consider using 1 decimal place for better accuracy, or conditionally show decimals for small percentages: {seg.percentage < 1 ? seg.percentage.toFixed(1) : seg.percentage.toFixed(0)}%4. Empty State Consistency (Very Low Priority)Issue: Empty state validation happens before percentage calculation but filters are applied earlier. Current observation: The logic is correct, but the flow could be slightly more explicit: // Current flow:
const validSegments = data.filter((d) => d.value > 0) // Line 967
const total = validSegments.reduce(...) // Line 980
if (total === 0) { /* empty state */ } // Line 984Suggestion: Add a comment clarifying why we check // Calculate total from non-zero segments
const total = validSegments.reduce((sum, d) => sum + d.value, 0)
// Edge case: Handle when all segments are zero or array is empty
if (total === 0) {5. Theme CSS Class Documentation (Low Priority)Issue: The base theme class Current code (theme.css:1111-1113): .gp-proportion-chart {
@apply relative w-full;
}Observation: The Suggestion: Add a comment or remove unused /* Base container for proportion chart - w-full for responsive behavior */
.gp-proportion-chart {
@apply w-full;
}🛡️ Security✅ No security concerns identified. The component:
⚡ Performance✅ Performance is good:
Minor note: For extremely large datasets (100+ segments), consider adding a prop to limit visible segments or group small ones into "Other", but this is likely not needed for typical use cases. 🧪 Testing✅ Excellent test coverage:
Optional enhancement: Consider adding a visual regression test or screenshot test if the repo uses those, but unit tests are comprehensive. 📋 Missing ElementsNone identified - the PR includes all necessary files:
🎯 RecommendationAPPROVE - This is high-quality work that's ready to merge. The suggestions above are minor enhancements that could be addressed in follow-up PRs if desired. The component is:
Great work on the migration from RibbonGraph with improvements! 🎉 Reviewed with Claude Code |
|
🧹 Preview deployment has been cleaned up. |
Summary
Adds ProportionChart component - a horizontal segmented bar chart for visualizing data proportions with interactive tooltips and optional legends.
Migrated from
RibbonGraphin gp-dixie with optimized redesign following gp-ui conventions.Features
Implementation Highlights
ProportionChartProps,ColorConfig,ProportionSegment)Testing
Demo
Comprehensive playground demo with 6 sections:
Files Added/Modified
New:
src/components/ProportionChart/ProportionChart.tsx(234 lines)src/components/ProportionChart/ProportionChart.test.tsx(257 lines)src/components/ProportionChart/index.tsplayground/src/components/ProportionChartDemo.tsx(271 lines)Modified:
src/components/index.ts- Export ProportionChartsrc/styles/theme.css- Add.gp-proportion-chartbase classplayground/src/App.tsx- Add to sidebar navigationUsage
```tsx
import { ProportionChart, type ColorConfig } from '@gp/ui'
const data = [
{ key: 'completed', value: 145 },
{ key: 'in-progress', value: 62 },
{ key: 'pending', value: 28 }
]
const colors: Record<string, ColorConfig> = {
completed: { bg: 'bg-green-500', label: 'Completed' },
'in-progress': { bg: 'bg-blue-500', label: 'In Progress' },
pending: { bg: 'bg-yellow-500', label: 'Pending' }
}
<ProportionChart
data={data}
colors={colors}
formatValue={(n) => `${n} tasks`}
height="h-4"
showLegend={true}
ariaLabel="Task distribution by status"
/>
```
🤖 Generated with Claude Code