Skip to content

feat: use full chart card for KPIs on member page #368

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.

Sign up for GitHub

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

Merged
drifter089 merged 1 commit into from
Jan 9, 2026
Merged

feat: use full chart card for KPIs on member page #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions src/app/member/[id]/_components/role-kpi-pair.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use client";

import { KpiCard } from "@/components/metric/kpi-card";
import { ReadOnlyMetricCard } from "@/app/dashboard/[teamId]/_components/dashboard-metric-card";
import { RoleCard, type RoleCardData } from "@/components/role/role-card";
import { cn } from "@/lib/utils";
import { type RouterOutputs } from "@/trpc/react";
Expand All @@ -11,14 +11,9 @@ type DashboardChart = RouterOutputs["dashboard"]["getDashboardCharts"][number];
interface RoleKpiPairProps {
role: Role;
dashboardChart?: DashboardChart;
teamId: string;
}

export function RoleKpiPair({
role,
dashboardChart,
teamId,
}: RoleKpiPairProps) {
export function RoleKpiPair({ role, dashboardChart }: RoleKpiPairProps) {
const roleCardData: RoleCardData = {
id: role.id,
title: role.title,
Expand All @@ -41,25 +36,18 @@ export function RoleKpiPair({
return (
<div
className={cn(
"grid items-stretch gap-3",
"grid items-stretch gap-4",
hasKpi || hasMetricButNoChart
? "grid-cols-1 lg:grid-cols-2"
: "grid-cols-1",
)}
>
<RoleCard role={roleCardData} readOnly className="h-full" />

{hasKpi && (
<KpiCard
dashboardChart={dashboardChart}
teamId={teamId}
showSettings={false}
enableDragDrop={false}
/>
)}
{hasKpi && <ReadOnlyMetricCard dashboardChart={dashboardChart} />}

{hasMetricButNoChart && (
<div className="border-border/60 text-muted-foreground flex items-center justify-center rounded-lg border border-dashed p-6">
<div className="border-border/60 text-muted-foreground flex h-[420px] items-center justify-center rounded-xl border border-dashed p-6">
Copy link

@coderabbitai coderabbitai bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find the location of ReadOnlyMetricCard and related components
fd -t f 'dashboard-metric' src/ | head -20
fd -t f 'role-kpi-pair' src/

Repository: drifter089/orgOS

Length of output: 374

🏁 Script executed:

# Search for ReadOnlyMetricCard definition
rg -l 'ReadOnlyMetricCard' src/

Repository: drifter089/orgOS

Length of output: 366

🏁 Script executed:

# Get more context about the file under review
cat -n src/app/member/\[id\]/_components/role-kpi-pair.tsx | head -60

Repository: drifter089/orgOS

Length of output: 2158

🏁 Script executed:

# Read the dashboard-metric-card.tsx file
cat -n src/app/dashboard/\[teamId\]/_components/dashboard-metric-card.tsx

Repository: drifter089/orgOS

Length of output: 6447

🏁 Script executed:

# Also check the DashboardMetricChart component since it's likely used by ReadOnlyMetricCard
cat -n src/app/dashboard/\[teamId\]/_components/dashboard-metric-chart.tsx

Repository: drifter089/orgOS

Length of output: 18496

Use a shared CSS variable for the metric card height instead of hardcoding h-[420px] in two places.

Both the placeholder in this file and ReadOnlyMetricCard (via DashboardMetricChart) currently use h-[420px], so heights match. However, this magic number is duplicated across the codebase. Extracting it to a CSS variable in your globals or theme config ensures future changes to the metric card height automatically propagate everywhere, preventing layout shifts during maintenance.

🤖 Prompt for AI Agents 
In @src/app/member/[id]/_components/role-kpi-pair.tsx at line 50, The
placeholder div uses a hardcoded Tailwind height class h-[420px]; replace this
with a CSS variable so the metric card height is shared with
ReadOnlyMetricCard/DashboardMetricChart: define --metric-card-height: 420px in
your global CSS or theme config (e.g., :root or globals.css) and change the JSX
className to use h-[var(--metric-card-height)] (and update the same replacement
inside ReadOnlyMetricCard / DashboardMetricChart) so both components read the
single source of truth for height.

<span className="text-sm">KPI data loading...</span>
</div>
)}
Expand Down
1 change: 0 additions & 1 deletion src/app/member/[id]/_components/team-section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ export function TeamSection({
? chartsByMetricId.get(role.metricId)
: undefined
}
teamId={team.id}
/>
))}
</div>
Expand Down
3 changes: 2 additions & 1 deletion src/components/role/role-card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ function RoleCardComponent({
return (
<div
className={cn(
"bg-card group relative flex flex-col rounded-lg border transition-all duration-200 hover:shadow-lg",
"bg-card group relative flex flex-col rounded-lg border transition-all duration-200",
!readOnly && "hover:shadow-lg",
selected && "ring-primary ring-2 ring-offset-2",
isPending && "opacity-70",
isDeleting && "opacity-50",
Expand Down