Address code review on mobile team breakdown
- Use the shared Radix Select primitive for the mobile sort control instead of a one-off native <select>, matching the design system. - Fix the sort-direction toggle aria-label to describe the action it performs rather than the current state. - Drop a redundant flex wrapper around PointsValue in the desktop cell. - Add jsdom stubs (scrollIntoView, pointer-capture) to the shared test setup so Radix Select can be exercised in unit tests, and update the mobile sort tests to drive the Radix dropdown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGQoRPjRycNSjwLrUwuKhw
This commit is contained in:
parent
215c37350b
commit
45c7950805
3 changed files with 41 additions and 19 deletions
|
|
@ -3,6 +3,13 @@ import { Link } from "react-router";
|
|||
import { ArrowUp, ArrowDown, ArrowUpDown } from "lucide-react";
|
||||
import { Button } from "~/components/ui/button";
|
||||
import { Card, CardContent } from "~/components/ui/card";
|
||||
import {
|
||||
Select,
|
||||
SelectContent,
|
||||
SelectItem,
|
||||
SelectTrigger,
|
||||
SelectValue,
|
||||
} from "~/components/ui/select";
|
||||
import { Table, TableHeader, TableRow, TableHead, TableBody, TableCell } from "~/components/ui/table";
|
||||
import { Badge } from "~/components/ui/badge";
|
||||
|
||||
|
|
@ -271,9 +278,7 @@ export function TeamScoreBreakdown({
|
|||
<PositionCell pick={pick} />
|
||||
</TableCell>
|
||||
<TableCell className="text-right">
|
||||
<div className="flex flex-col items-end">
|
||||
<PointsValue pick={pick} />
|
||||
</div>
|
||||
<PointsValue pick={pick} />
|
||||
</TableCell>
|
||||
</TableRow>
|
||||
))}
|
||||
|
|
@ -286,26 +291,29 @@ export function TeamScoreBreakdown({
|
|||
<div className="md:hidden space-y-3" data-testid="picks-mobile">
|
||||
{/* Sort control */}
|
||||
<div className="flex items-center gap-2">
|
||||
<label htmlFor="picks-sort" className="text-sm text-muted-foreground shrink-0">
|
||||
<span id="picks-sort-label" className="text-sm text-muted-foreground shrink-0">
|
||||
Sort by
|
||||
</label>
|
||||
<select
|
||||
id="picks-sort"
|
||||
</span>
|
||||
<Select
|
||||
value={sortColumn}
|
||||
onChange={(e) => handleSortColumn(e.target.value as SortColumn)}
|
||||
className="flex-1 rounded-md border border-input bg-transparent px-3 py-2 text-sm shadow-xs outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50"
|
||||
onValueChange={(v) => handleSortColumn(v as SortColumn)}
|
||||
>
|
||||
{SORT_OPTIONS.map((opt) => (
|
||||
<option key={opt.value} value={opt.value}>
|
||||
{opt.label}
|
||||
</option>
|
||||
))}
|
||||
</select>
|
||||
<SelectTrigger className="flex-1" aria-labelledby="picks-sort-label">
|
||||
<SelectValue />
|
||||
</SelectTrigger>
|
||||
<SelectContent>
|
||||
{SORT_OPTIONS.map((opt) => (
|
||||
<SelectItem key={opt.value} value={opt.value}>
|
||||
{opt.label}
|
||||
</SelectItem>
|
||||
))}
|
||||
</SelectContent>
|
||||
</Select>
|
||||
<Button
|
||||
variant="outline"
|
||||
size="icon"
|
||||
onClick={() => setSortDirection(sortDirection === "asc" ? "desc" : "asc")}
|
||||
aria-label={`Sort ${sortDirection === "asc" ? "ascending" : "descending"}`}
|
||||
aria-label={`Sort ${sortDirection === "asc" ? "descending" : "ascending"}`}
|
||||
>
|
||||
{sortDirection === "asc" ? (
|
||||
<ArrowUp className="h-4 w-4" />
|
||||
|
|
|
|||
|
|
@ -519,6 +519,11 @@ describe("TeamScoreBreakdown", () => {
|
|||
);
|
||||
}
|
||||
|
||||
async function selectSortColumn(user: ReturnType<typeof userEvent.setup>, label: string) {
|
||||
await user.click(screen.getByRole("combobox", { name: /sort by/i }));
|
||||
await user.click(screen.getByRole("option", { name: label }));
|
||||
}
|
||||
|
||||
it("renders a card per pick with all fields visible", () => {
|
||||
renderWithRouter(
|
||||
<TeamScoreBreakdown
|
||||
|
|
@ -571,7 +576,7 @@ describe("TeamScoreBreakdown", () => {
|
|||
);
|
||||
|
||||
// picks: Team A=100, Driver B=50, Team C=0; points defaults to descending
|
||||
await user.selectOptions(screen.getByLabelText(/sort by/i), "points");
|
||||
await selectSortColumn(user, "Points");
|
||||
expect(getCardOrder()).toEqual(["Team A", "Driver B", "Team C"]);
|
||||
});
|
||||
|
||||
|
|
@ -587,8 +592,9 @@ describe("TeamScoreBreakdown", () => {
|
|||
/>
|
||||
);
|
||||
|
||||
await user.selectOptions(screen.getByLabelText(/sort by/i), "points"); // desc
|
||||
await user.click(screen.getByRole("button", { name: /sort descending/i }));
|
||||
await selectSortColumn(user, "Points"); // desc
|
||||
// While descending, the toggle's action is to sort ascending.
|
||||
await user.click(screen.getByRole("button", { name: /sort ascending/i }));
|
||||
// now ascending: Team C (0), Driver B (50), Team A (100)
|
||||
expect(getCardOrder()).toEqual(["Team C", "Driver B", "Team A"]);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -29,6 +29,14 @@ process.env.NODE_ENV = 'test';
|
|||
// jsdom does not implement scrollTo on elements
|
||||
HTMLElement.prototype.scrollTo = vi.fn();
|
||||
|
||||
// jsdom does not implement these APIs that Radix UI relies on for pointer
|
||||
// interactions and option positioning (Select, DropdownMenu, etc.). Without
|
||||
// them, opening a Radix Select in a test throws.
|
||||
HTMLElement.prototype.scrollIntoView = vi.fn();
|
||||
HTMLElement.prototype.hasPointerCapture = vi.fn(() => false);
|
||||
HTMLElement.prototype.setPointerCapture = vi.fn();
|
||||
HTMLElement.prototype.releasePointerCapture = vi.fn();
|
||||
|
||||
// Mock BetterAuth
|
||||
vi.mock('~/lib/auth.server', () => ({
|
||||
auth: {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue