fix: improve KPI card layout with compact color pill and full-width progress bars #363
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe KPI card component undergoes a comprehensive UI refactor, replacing the multi-column layout with a streamlined header design, restructuring action controls into icon-only toggles, consolidating progress visualization into a single horizontal bar, and removing five canvas-specific properties from the public API while retaining core functionality through internal defaults. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Code Review: KPI Card Layout ImprovementsOverall Assessment ✅This is a solid UI improvement PR with clean refactoring. The changes improve visual hierarchy and code readability. A few minor considerations below. Strengths 💪
Potential Issues 🔍1. Progress Bar Height Change (Minor)// Changed from h-1 (4px) to h-1.5 (6px)
<div className="bg-muted h-1.5 flex-1 overflow-hidden rounded-full">Impact: 50% height increase. While the PR description mentions "better visibility", this might affect density on dashboards with many KPI cards. Recommendation: Verify this looks good in dense dashboard views with 6+ cards. 2. Time Remaining Display Removed (Medium)Before: Showed "3d left" along with time progress percentage - <span className="text-muted-foreground">
- {formatTimeRemaining(goalProgress)}
- </span>
- <span className="text-muted-foreground">·</span>
- <span className="text-muted-foreground capitalize">
- {formatCadence(goalProgress.cadence)}
- </span>
+ <span className="text-muted-foreground shrink-0">
+ {formatTimeRemaining(goalProgress)}
+ </span>Impact: Users now see "Weekly Goal" at the top but lose the "3d left" label next to the time progress bar. The time remaining info is still shown but without the cadence redundancy. Recommendation: This seems intentional based on PR description. If users relied on seeing both together, might need adjustment. Otherwise, looks good! 3. Missing Comment Documentation (Low)The PR removes the JSDoc comment for -/**
- * Format time remaining based on cadence
- * - DAILY or < 1 day: show hours (e.g., "8h left")
- * - WEEKLY/MONTHLY: show days (e.g., "3d left")
- */
function formatTimeRemaining(goalProgress: GoalProgress): string {Per CLAUDE.md: "Add comments in code where necessary" Recommendation: The logic is fairly self-evident, but the comment explained the < 1 day threshold behavior. Consider keeping it or ensure the behavior is obvious from code. Code Quality Observations 👀1. Non-null Assertions// Line 240, 243
style={{ backgroundColor: roles[0]!.color }}
<span>{roles[0]!.title}</span>Status: This is safe since it's wrapped in 2. Color Pill Height Hardcoded<div className={cn("h-8 w-1.5 shrink-0 rounded-sm", ...)} />Consideration: The Current Status: Acceptable since the design shows fixed-height cards. Just something to note if titles can wrap in the future. 3. Consistent Use of
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/metric/kpi-card.tsx (1)
187-232: Consider adding accessibility attributes to progress bars.The progress bars lack proper ARIA attributes for screen readers. Consider adding
role="progressbar",aria-valuenow,aria-valuemin="0",aria-valuemax="100", andaria-labelto the outer progress bar containers to improve accessibility.♿ Example implementation
For the goal progress bar:
-<div className="bg-muted h-2 flex-1 overflow-hidden rounded-full"> +<div + className="bg-muted h-2 flex-1 overflow-hidden rounded-full" + role="progressbar" + aria-valuenow={Math.round(goalProgress.progressPercent)} + aria-valuemin={0} + aria-valuemax={100} + aria-label="Goal progress" +>Apply similar changes to the time progress bar with an appropriate label like "Time elapsed".
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/metric/kpi-card.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
Use TypeScript 5.9 with strict type checking for all frontend and backend code
Files:
src/components/metric/kpi-card.tsx
src/**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Prefer Server Components for initial data fetching; use Client Components ('use client') only for interactivity
Files:
src/components/metric/kpi-card.tsx
src/**/*/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Client Components must use
import { api } from '@/trpc/react'for standard HTTP/Hooks wrapper
Files:
src/components/metric/kpi-card.tsx
src/components/**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Place colocated components in
_components/folders next to their parent componentUse shadcn/ui components from src/components/ui/; add new components via CLI: npx shadcn@latest add [component-name]
Files:
src/components/metric/kpi-card.tsx
**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Use Tailwind CSS 4 for styling with shadcn/ui and Radix UI primitive components
Files:
src/components/metric/kpi-card.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use @trivago/prettier-plugin-sort-imports with inline type imports for import organization
Files:
src/components/metric/kpi-card.tsx
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/components/dashboard-metric-card.tsx,src/app/dashboard/[teamId]/_components/public-dashboard-metric-card.tsx : Dashboard metric cards are duplicated with public variant. Consolidate into single component with `readOnly` mode prop instead of maintaining separate components.
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/components/dashboard-metric-card.tsx,src/app/dashboard/[teamId]/_components/public-dashboard-metric-card.tsx : Dashboard metric cards are duplicated with public variant. Consolidate into single component with `readOnly` mode prop instead of maintaining separate components.
Applied to files:
src/components/metric/kpi-card.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/(metric|dashboard)/**/*.tsx : Use three-stage metrics transformation: API → DataPoints (DataIngestionTransformer), DataPoints → ChartConfig (ChartTransformer), ChartConfig → UI (DashboardMetricChart)
Applied to files:
src/components/metric/kpi-card.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Metric dialog components have nearly identical wrapper patterns (5 files). Consider implementing a factory pattern or generic wrapper to reduce duplication across provider dialogs.
Applied to files:
src/components/metric/kpi-card.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Use shared MetricDialogBase component from base/ for all metric dialog implementations
Applied to files:
src/components/metric/kpi-card.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Metric dialog components should follow the pattern: [Provider]MetricDialog.tsx (dialog wrapper) and [Provider]MetricContent.tsx (form content). Register in src/app/metric/_components/index.ts and inherit from shared MetricDialogBase.
Applied to files:
src/components/metric/kpi-card.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/hooks/**/*.ts : For role-metric cache updates, optimistically update both role cache (role.getByTeamId) and dashboard cache (dashboard.getDashboardCharts) to maintain UI consistency
Applied to files:
src/components/metric/kpi-card.tsx
📚 Learning: 2025-12-20T22:12:00.576Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-12-20T22:12:00.576Z
Learning: The Metrics Pipeline processes data through 3 stages: DataIngestionTransformer (Raw API → MetricDataPoint), ChartTransformer (MetricDataPoint → Chart Config), and Visualization (Chart Config → Recharts UI)
Applied to files:
src/components/metric/kpi-card.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (1)
src/components/metric/kpi-card.tsx (1)
29-39: Inconsistency between AI summary and actual code.The AI-generated summary claims that
isOnCanvas,isDragging,onDragStart,onDragEnd, andonToggleVisibilitywere removed from the public interface, but these properties are still present in theKpiCardPropsinterface (lines 34-38). Please verify whether these props should be retained or removed from the public API.
Summary
Improves the KPI card layout based on feedback: shorter color pill aligned with header only, cadence shown on separate line, and full-width progress bars.
Key Changes
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.