Code review fixes: module-level comparators, correct type on SortableHead, handle negative point change, remove redundant spread
- Move comparators object to module level (closes over nothing, no useMemo needed) - Use SortConfig<StandingRow> on SortableHead instead of inline duplicate type - SevenDayChange: handle negative pointChange with correct sign and color - Remove redundant sevenDayPointChange explicit assignment (already in ...standing spread) https://claude.ai/code/session_01CuCKFVYbpsKSQoFDcTYfY7
This commit is contained in:
parent
0b74a6a2d2
commit
5e572d6351
2 changed files with 33 additions and 44 deletions
|
|
@ -3,7 +3,7 @@ import { Table, TableHeader, TableRow, TableHead, TableBody, TableCell } from "~
|
||||||
import { Badge } from "~/components/ui/badge";
|
import { Badge } from "~/components/ui/badge";
|
||||||
import { TeamNameDisplay } from "~/components/ui/team-name-display";
|
import { TeamNameDisplay } from "~/components/ui/team-name-display";
|
||||||
import { PointsDisplay } from "~/components/standings/PointsDisplay";
|
import { PointsDisplay } from "~/components/standings/PointsDisplay";
|
||||||
import { useSortableData, type SortDirection } from "~/hooks/useSortableData";
|
import { useSortableData, type SortConfig, type SortDirection } from "~/hooks/useSortableData";
|
||||||
import { type TeamStanding } from "~/types/standings";
|
import { type TeamStanding } from "~/types/standings";
|
||||||
|
|
||||||
interface StandingsTableProps {
|
interface StandingsTableProps {
|
||||||
|
|
@ -17,6 +17,34 @@ interface StandingRow extends TeamStanding {
|
||||||
_sortPoints: number;
|
_sortPoints: number;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Comparators are pure functions with no component state — define at module level
|
||||||
|
const comparators = {
|
||||||
|
// Rank: ties broken alphabetically by team name regardless of sort direction
|
||||||
|
currentRank: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
||||||
|
const cmp = a.currentRank - b.currentRank;
|
||||||
|
if (cmp !== 0) return dir === "asc" ? cmp : -cmp;
|
||||||
|
return a.teamName.localeCompare(b.teamName);
|
||||||
|
},
|
||||||
|
_sortPoints: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
||||||
|
const cmp = b._sortPoints - a._sortPoints;
|
||||||
|
return dir === "asc" ? -cmp : cmp;
|
||||||
|
},
|
||||||
|
sevenDayPointChange: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
||||||
|
const aVal = a.sevenDayPointChange ?? 0;
|
||||||
|
const bVal = b.sevenDayPointChange ?? 0;
|
||||||
|
const cmp = bVal - aVal;
|
||||||
|
return dir === "asc" ? -cmp : cmp;
|
||||||
|
},
|
||||||
|
teamName: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
||||||
|
const cmp = a.teamName.localeCompare(b.teamName);
|
||||||
|
return dir === "asc" ? cmp : -cmp;
|
||||||
|
},
|
||||||
|
participantsRemaining: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
||||||
|
const cmp = a.participantsRemaining - b.participantsRemaining;
|
||||||
|
return dir === "asc" ? cmp : -cmp;
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Display team standings with ranking, points, 7-day change, and sortable columns.
|
* Display team standings with ranking, points, 7-day change, and sortable columns.
|
||||||
*/
|
*/
|
||||||
|
|
@ -30,42 +58,6 @@ export function StandingsTable({ standings, leagueId, seasonId }: StandingsTable
|
||||||
[standings]
|
[standings]
|
||||||
);
|
);
|
||||||
|
|
||||||
// Custom comparators
|
|
||||||
const comparators = useMemo(
|
|
||||||
() => ({
|
|
||||||
// Rank: ascending by rank, ties broken alphabetically by team name
|
|
||||||
currentRank: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
|
||||||
const cmp = a.currentRank - b.currentRank;
|
|
||||||
if (cmp !== 0) return dir === "asc" ? cmp : -cmp;
|
|
||||||
// Within ties, always sort by team name A→Z regardless of direction
|
|
||||||
return a.teamName.localeCompare(b.teamName);
|
|
||||||
},
|
|
||||||
// Points: descending by default means best score first
|
|
||||||
_sortPoints: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
|
||||||
const cmp = b._sortPoints - a._sortPoints;
|
|
||||||
return dir === "asc" ? -cmp : cmp;
|
|
||||||
},
|
|
||||||
// 7-day point change: higher change = better, descending first
|
|
||||||
sevenDayPointChange: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
|
||||||
const aVal = a.sevenDayPointChange ?? 0;
|
|
||||||
const bVal = b.sevenDayPointChange ?? 0;
|
|
||||||
const cmp = bVal - aVal;
|
|
||||||
return dir === "asc" ? -cmp : cmp;
|
|
||||||
},
|
|
||||||
// Team name: A→Z ascending
|
|
||||||
teamName: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
|
||||||
const cmp = a.teamName.localeCompare(b.teamName);
|
|
||||||
return dir === "asc" ? cmp : -cmp;
|
|
||||||
},
|
|
||||||
// Remaining: fewer remaining = closer to done, ascending = most remaining first
|
|
||||||
participantsRemaining: (a: StandingRow, b: StandingRow, dir: SortDirection) => {
|
|
||||||
const cmp = a.participantsRemaining - b.participantsRemaining;
|
|
||||||
return dir === "asc" ? cmp : -cmp;
|
|
||||||
},
|
|
||||||
}),
|
|
||||||
[]
|
|
||||||
);
|
|
||||||
|
|
||||||
const { sortedData, sortConfig, requestSort } = useSortableData<StandingRow>(
|
const { sortedData, sortConfig, requestSort } = useSortableData<StandingRow>(
|
||||||
rows,
|
rows,
|
||||||
"currentRank",
|
"currentRank",
|
||||||
|
|
@ -187,8 +179,8 @@ export function StandingsTable({ standings, leagueId, seasonId }: StandingsTable
|
||||||
|
|
||||||
interface SortableHeadProps {
|
interface SortableHeadProps {
|
||||||
label: React.ReactNode;
|
label: React.ReactNode;
|
||||||
sortKey: string;
|
sortKey: keyof StandingRow;
|
||||||
sortConfig: { key: string | null; direction: "asc" | "desc" };
|
sortConfig: SortConfig<StandingRow>;
|
||||||
onSort: () => void;
|
onSort: () => void;
|
||||||
className?: string;
|
className?: string;
|
||||||
}
|
}
|
||||||
|
|
@ -278,11 +270,9 @@ function SevenDayChange({
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div className="flex flex-col items-end gap-0.5">
|
<div className="flex flex-col items-end gap-0.5">
|
||||||
{/* Point change — always non-negative */}
|
<span className={`text-sm font-medium ${pointChange >= 0 ? "text-emerald-400" : "text-coral-accent"}`}>
|
||||||
<span className="text-sm font-medium text-emerald-400">
|
{pointChange >= 0 ? "+" : ""}{pointChange.toFixed(2)} pts
|
||||||
+{pointChange.toFixed(2)} pts
|
|
||||||
</span>
|
</span>
|
||||||
{/* Rank change */}
|
|
||||||
{rankChange > 0 ? (
|
{rankChange > 0 ? (
|
||||||
<span className="text-xs text-emerald-400">↑{rankChange} rank</span>
|
<span className="text-xs text-emerald-400">↑{rankChange} rank</span>
|
||||||
) : rankChange < 0 ? (
|
) : rankChange < 0 ? (
|
||||||
|
|
|
||||||
|
|
@ -102,7 +102,6 @@ export async function loader(args: Route.LoaderArgs) {
|
||||||
const formattedStandings = standingsWithComparison.map((standing) => ({
|
const formattedStandings = standingsWithComparison.map((standing) => ({
|
||||||
...standing,
|
...standing,
|
||||||
rankChange: standing.sevenDayRankChange,
|
rankChange: standing.sevenDayRankChange,
|
||||||
sevenDayPointChange: standing.sevenDayPointChange,
|
|
||||||
ownerName: ownerNameByTeamId.get(standing.teamId) ?? null,
|
ownerName: ownerNameByTeamId.get(standing.teamId) ?? null,
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue