refactor: address code review findings in Roster/Summary draft views (#57)

- Extract groupPicksByTeamAndSport into shared picks-utils.ts to remove
  duplication between TeamRosterView and DraftSummaryView
- Fix TeamRosterView: make teamPicks a proper useMemo dep on selectedTeamId,
  add useEffect to reset stale selection if draftSlots changes
- Add accessible label/htmlFor to TeamRosterView team Select
- Remove dead `season` prop from DraftSummaryView (was inherited from
  TeamsDraftedGrid but never used)
- Fix DraftSummaryView: sport name cells are now <th scope="row">, column
  headers have scope="col", Total header uses solid bg-muted to match Sport
- Replace || null with explicit > 0 check in Total cell for clarity
- Consolidate to summaryViewProps/rosterViewProps in route for consistency

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Chris Parsons 2026-03-02 16:46:35 -08:00 committed by GitHub
parent 46332ea735
commit 599bba8949
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 309 additions and 14 deletions

View file

@ -0,0 +1,114 @@
import { memo, useMemo } from "react";
import { type DraftPick, groupPicksByTeamAndSport } from "./picks-utils";
interface DraftSummaryViewProps {
draftSlots: Array<{ id: string; team: { id: string; name: string } }>;
ownerMap?: Record<string, string>;
picks: DraftPick[];
sports: Array<{ id: string; name: string }>;
}
export const DraftSummaryView = memo(function DraftSummaryView({
draftSlots,
ownerMap = {},
picks,
sports,
}: DraftSummaryViewProps) {
const picksByTeamAndSport = useMemo(
() => groupPicksByTeamAndSport(picks),
[picks]
);
const totalPicksBySport = useMemo(() => {
const map = new Map<string, number>();
sports.forEach((sport) => {
let count = 0;
picksByTeamAndSport.forEach((teamMap) => {
count += teamMap.get(sport.id)?.length ?? 0;
});
map.set(sport.id, count);
});
return map;
}, [picksByTeamAndSport, sports]);
if (sports.length === 0) {
return (
<div className="flex items-center justify-center h-full p-8 text-muted-foreground">
<p>No sports have been configured for this season.</p>
</div>
);
}
return (
<div className="w-full h-full overflow-auto">
<table className="w-full border-collapse">
<thead className="sticky top-0 bg-background z-10">
<tr>
<th
scope="col"
className="border-r border-b border-border p-2 text-left font-semibold bg-muted sticky left-0 z-20 min-w-[120px]"
>
Sport
</th>
<th
scope="col"
className="border-r border-b border-border p-2 text-center font-semibold bg-muted min-w-[48px]"
>
Total
</th>
{draftSlots.map((slot, index) => {
const displayName = ownerMap[slot.team.id] || slot.team.name;
const isLast = index === draftSlots.length - 1;
return (
<th
key={slot.id}
scope="col"
className={`border-b border-border p-2 text-center font-semibold bg-muted/50 min-w-[60px] ${!isLast ? "border-r" : ""}`}
>
<span className="text-sm">{displayName}</span>
</th>
);
})}
</tr>
</thead>
<tbody>
{sports.map((sport, sportIndex) => {
const isLastRow = sportIndex === sports.length - 1;
const sportTotal = totalPicksBySport.get(sport.id) ?? 0;
return (
<tr key={sport.id}>
<th
scope="row"
className={`border-r border-border p-2 font-medium text-sm text-left bg-muted sticky left-0 z-10 ${!isLastRow ? "border-b" : ""}`}
>
{sport.name}
</th>
<td
className={`border-r border-border p-2 text-center text-sm font-semibold bg-muted/30 ${!isLastRow ? "border-b" : ""}`}
>
{sportTotal > 0 ? sportTotal : null}
</td>
{draftSlots.map((slot, slotIndex) => {
const count =
picksByTeamAndSport.get(slot.team.id)?.get(sport.id)
?.length ?? 0;
const isLast = slotIndex === draftSlots.length - 1;
return (
<td
key={`${slot.team.id}-${sport.id}`}
className={`border-border p-2 text-center text-sm ${
count >= 2 ? "bg-accent/30 font-semibold" : "bg-background"
} ${!isLastRow ? "border-b" : ""} ${!isLast ? "border-r" : ""}`}
>
{count > 0 ? count : null}
</td>
);
})}
</tr>
);
})}
</tbody>
</table>
</div>
);
});

View file

@ -0,0 +1,121 @@
import { memo, useEffect, useMemo, useState } from "react";
import { Badge } from "~/components/ui/badge";
import {
Select,
SelectContent,
SelectItem,
SelectTrigger,
SelectValue,
} from "~/components/ui/select";
import { type DraftPick, groupPicksByTeamAndSport } from "./picks-utils";
interface TeamRosterViewProps {
draftSlots: Array<{ id: string; team: { id: string; name: string } }>;
ownerMap?: Record<string, string>;
picks: DraftPick[];
sports: Array<{ id: string; name: string }>;
}
export const TeamRosterView = memo(function TeamRosterView({
draftSlots,
ownerMap = {},
picks,
sports,
}: TeamRosterViewProps) {
const [selectedTeamId, setSelectedTeamId] = useState(
draftSlots[0]?.team.id ?? ""
);
// Reset selection if the selected team is no longer present in the slot list
useEffect(() => {
if (
draftSlots.length > 0 &&
!draftSlots.some((s) => s.team.id === selectedTeamId)
) {
setSelectedTeamId(draftSlots[0].team.id);
}
}, [draftSlots, selectedTeamId]);
const picksByTeamAndSport = useMemo(
() => groupPicksByTeamAndSport(picks),
[picks]
);
const teamPicks = useMemo(
() => picksByTeamAndSport.get(selectedTeamId),
[picksByTeamAndSport, selectedTeamId]
);
const sportsWithPicks = useMemo(
() => sports.filter((s) => (teamPicks?.get(s.id)?.length ?? 0) > 0),
[sports, teamPicks]
);
const totalPicks = useMemo(() => {
let count = 0;
teamPicks?.forEach((sp) => (count += sp.length));
return count;
}, [teamPicks]);
if (draftSlots.length === 0) {
return (
<div className="flex items-center justify-center h-full p-8 text-muted-foreground">
<p>No teams in this draft.</p>
</div>
);
}
return (
<div className="h-full overflow-y-auto">
<div className="p-4 space-y-4">
<div className="space-y-1">
<label
htmlFor="roster-team-select"
className="text-xs font-medium text-muted-foreground"
>
Team
</label>
<Select value={selectedTeamId} onValueChange={setSelectedTeamId}>
<SelectTrigger id="roster-team-select" className="w-full">
<SelectValue placeholder="Select a team" />
</SelectTrigger>
<SelectContent>
{draftSlots.map((slot) => (
<SelectItem key={slot.id} value={slot.team.id}>
{ownerMap[slot.team.id] || slot.team.name}
</SelectItem>
))}
</SelectContent>
</Select>
</div>
{totalPicks === 0 ? (
<p className="text-sm text-muted-foreground text-center py-6">
No picks yet
</p>
) : (
sportsWithPicks.map((sport) => {
const sportPicks = teamPicks?.get(sport.id) ?? [];
return (
<div key={sport.id}>
<div className="flex items-center gap-2 mb-2">
<h3 className="text-sm font-semibold">{sport.name}</h3>
<Badge variant="secondary" className="text-xs px-1.5 py-0">
{sportPicks.length}
</Badge>
</div>
<ul className="space-y-1 pl-2">
{sportPicks.map((pick) => (
<li key={pick.id} className="text-sm text-foreground">
{pick.participant.name}
</li>
))}
</ul>
</div>
);
})
)}
</div>
</div>
);
});

View file

@ -0,0 +1,19 @@
export type DraftPick = {
id: string;
team: { id: string; name: string };
participant: { id: string; name: string };
sport: { id: string; name: string };
};
export function groupPicksByTeamAndSport(
picks: DraftPick[]
): Map<string, Map<string, DraftPick[]>> {
const map = new Map<string, Map<string, DraftPick[]>>();
picks.forEach((pick) => {
if (!map.has(pick.team.id)) map.set(pick.team.id, new Map());
const teamMap = map.get(pick.team.id)!;
if (!teamMap.has(pick.sport.id)) teamMap.set(pick.sport.id, []);
teamMap.get(pick.sport.id)!.push(pick);
});
return map;
}

View file

@ -14,7 +14,8 @@ import { getAuth } from "@clerk/react-router/server";
import { getTeamQueue } from "~/models/draft-queue";
import { buildOwnerMap } from "~/lib/owner-map";
import { DraftSidebar } from "~/components/DraftSidebar";
import { TeamsDraftedGrid } from "~/components/draft/TeamsDraftedGrid";
import { TeamRosterView } from "~/components/draft/TeamRosterView";
import { DraftSummaryView } from "~/components/draft/DraftSummaryView";
import { QueueSection } from "~/components/draft/QueueSection";
import { AvailableParticipantsSection } from "~/components/draft/AvailableParticipantsSection";
import { SidebarRecentPicks } from "~/components/draft/SidebarRecentPicks";
@ -37,7 +38,7 @@ type QueueItem = typeof schema.draftQueue.$inferSelect;
const MOBILE_TABS_BASE = [
{ id: "available" as const, label: "Available", Icon: Users },
{ id: "board" as const, label: "Board", Icon: LayoutGrid },
{ id: "roster" as const, label: "Roster", Icon: ListChecks },
{ id: "teams" as const, label: "Teams", Icon: ListChecks },
{ id: "controls" as const, label: "Controls", Icon: Settings },
];
@ -466,10 +467,11 @@ export default function DraftRoom() {
const stored = localStorage.getItem("draftSidebarCollapsed");
return stored ? JSON.parse(stored) : false;
});
const [activeTab, setActiveTab] = useState<"participants" | "board" | "teams">("board");
const [mobileTab, setMobileTab] = useState<"available" | "queue" | "board" | "roster" | "controls">(
const [activeTab, setActiveTab] = useState<"participants" | "board" | "rosters" | "summary">("board");
const [mobileTab, setMobileTab] = useState<"available" | "queue" | "board" | "teams" | "controls">(
!userTeam && isCommissioner ? "board" : "available"
);
const [teamsSubTab, setTeamsSubTab] = useState<"rosters" | "summary">("rosters");
const isMobile = useMediaQuery("(max-width: 767px)");
// Track autodraft status for all teams
@ -1395,13 +1397,17 @@ export default function DraftRoom() {
onRollbackToPick: isCommissioner && !isDraftComplete ? handleRollbackToPick : undefined,
};
const teamsDraftedGridSeason = useMemo(() => ({ numFlexPicks }), [numFlexPicks]);
const teamsDraftedGridProps = {
const summaryViewProps = {
draftSlots,
picks,
sports: seasonSportsData,
ownerMap,
};
const rosterViewProps = {
draftSlots,
picks,
sports: seasonSportsData,
season: teamsDraftedGridSeason,
ownerMap,
};
@ -1584,8 +1590,38 @@ export default function DraftRoom() {
{mobileTab === "board" && (
<DraftGridSection {...draftGridSectionProps} />
)}
{mobileTab === "roster" && (
<TeamsDraftedGrid {...teamsDraftedGridProps} />
{mobileTab === "teams" && (
<div className="flex flex-col h-full overflow-hidden">
<div className="flex-shrink-0 flex gap-1 p-2 border-b">
<button
onClick={() => setTeamsSubTab("rosters")}
className={`flex-1 py-1.5 text-sm font-medium rounded transition-colors ${
teamsSubTab === "rosters"
? "bg-accent text-accent-foreground"
: "text-muted-foreground hover:text-foreground"
}`}
>
Roster
</button>
<button
onClick={() => setTeamsSubTab("summary")}
className={`flex-1 py-1.5 text-sm font-medium rounded transition-colors ${
teamsSubTab === "summary"
? "bg-accent text-accent-foreground"
: "text-muted-foreground hover:text-foreground"
}`}
>
Summary
</button>
</div>
<div className="flex-1 overflow-hidden">
{teamsSubTab === "rosters" ? (
<TeamRosterView {...rosterViewProps} />
) : (
<DraftSummaryView {...summaryViewProps} />
)}
</div>
</div>
)}
{mobileTab === "controls" && (
<div className="h-full overflow-y-auto p-4 space-y-6">
@ -1649,7 +1685,7 @@ export default function DraftRoom() {
<Tabs
value={activeTab}
onValueChange={(value) =>
setActiveTab(value as "participants" | "board" | "teams")
setActiveTab(value as "participants" | "board" | "rosters" | "summary")
}
className="h-full flex flex-col"
>
@ -1662,7 +1698,8 @@ export default function DraftRoom() {
<TabsList>
<TabsTrigger value="participants">Available Participants</TabsTrigger>
<TabsTrigger value="board">Draft Board</TabsTrigger>
<TabsTrigger value="teams">Teams Drafted</TabsTrigger>
<TabsTrigger value="rosters">Rosters</TabsTrigger>
<TabsTrigger value="summary">Summary</TabsTrigger>
</TabsList>
{season.status === "draft" && !isDraftComplete && currentDraftSlot && (
@ -1702,8 +1739,12 @@ export default function DraftRoom() {
<DraftGridSection {...draftGridSectionProps} />
</TabsContent>
<TabsContent value="teams" className="flex-1 overflow-hidden m-0">
<TeamsDraftedGrid {...teamsDraftedGridProps} />
<TabsContent value="rosters" className="flex-1 overflow-hidden m-0">
<TeamRosterView {...rosterViewProps} />
</TabsContent>
<TabsContent value="summary" className="flex-1 overflow-hidden m-0">
<DraftSummaryView {...summaryViewProps} />
</TabsContent>
</Tabs>
</div>