From 4326a46fd4e0bd05ba8892ea553f13c2b3448e1f Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Sun, 9 Aug 2026 02:08:26 +0000 Subject: [PATCH] questionFeed: WTC design repass rules 1/2/3/5/6 - uniform buttons, tap guard, landscape art frame, chip pruning, shared artist credit (#704) --- .../AttributeChipPanel.test.tsx | 82 ++++++++++++++++ .../attributeChips/AttributeChipPanel.tsx | 26 ++++- .../attributeChips/attributeChipRender.tsx | 19 ++-- .../attributeChips/attributeChips.test.ts | 41 ++++++++ .../features/attributeChips/attributeChips.ts | 31 ++++++ .../src/features/printingTags/cardPanel.tsx | 4 + .../questionFeed/QuestionFeed.test.tsx | 36 +++++++ .../features/questionFeed/QuestionFeed.tsx | 94 +++++++++++++++---- 8 files changed, 300 insertions(+), 33 deletions(-) diff --git a/frontend/src/features/attributeChips/AttributeChipPanel.test.tsx b/frontend/src/features/attributeChips/AttributeChipPanel.test.tsx index 43a970810..b612e93af 100644 --- a/frontend/src/features/attributeChips/AttributeChipPanel.test.tsx +++ b/frontend/src/features/attributeChips/AttributeChipPanel.test.tsx @@ -20,9 +20,11 @@ function buildRoute(path: string): string { function Wrapper({ store, onRateLimited, + pruneContradicted, }: { store: AppStore; onRateLimited?: () => void; + pruneContradicted?: boolean; }) { const [states, setStates] = React.useState(initialChipStates()); return ( @@ -35,6 +37,7 @@ function Wrapper({ onChipStatesChange={setStates} cardSlot={
card
} onRateLimited={onRateLimited} + pruneContradicted={pruneContradicted} /> ); @@ -124,6 +127,85 @@ describe("AttributeChipPanel", () => { expect(sibling.getAttribute("data-chip-state")).toBe("untouched"); }); + it("with pruneContradicted, disqualifies untouched exclusion-group siblings entirely", async () => { + server.use( + http.post(buildRoute("2/submitTagVote/"), async ({ request }) => { + const body = (await request.json()) as { + tagName: string; + polarity: number; + }; + return HttpResponse.json( + { + tagName: body.tagName, + resolvedPolarity: null, + netPolarity: 1, + tally: [], + }, + { status: 200 } + ); + }) + ); + render(); + + fireEvent.click(screen.getByTestId("attribute-chip-Black Border-yes")); + await waitFor(() => + expect( + screen + .getByTestId("attribute-chip-Black Border") + .getAttribute("data-chip-state") + ).toBe("positive") + ); + + // the positive chip stays; its untouched group-mates are hidden, not dimmed + expect(screen.getByTestId("attribute-chip-Black Border")).toBeVisible(); + expect(screen.queryByTestId("attribute-chip-White Border")).toBeNull(); + expect(screen.queryByTestId("attribute-chip-Silver Border")).toBeNull(); + // standalone chips are never contradicted and stay visible + expect(screen.getByTestId("attribute-chip-Full Art")).toBeVisible(); + }); + + it("with pruneContradicted, retracting the positive restores the hidden siblings", async () => { + server.use( + http.post(buildRoute("2/submitTagVote/"), async ({ request }) => { + const body = (await request.json()) as { + tagName: string; + polarity: number; + }; + return HttpResponse.json( + { + tagName: body.tagName, + resolvedPolarity: null, + netPolarity: body.polarity, + tally: [], + }, + { status: 200 } + ); + }) + ); + render(); + + fireEvent.click(screen.getByTestId("attribute-chip-Black Border-yes")); + await waitFor(() => + expect( + screen + .getByTestId("attribute-chip-Black Border") + .getAttribute("data-chip-state") + ).toBe("positive") + ); + expect(screen.queryByTestId("attribute-chip-White Border")).toBeNull(); + + // tapping the already-active Yes retracts to untouched - the siblings reappear + fireEvent.click(screen.getByTestId("attribute-chip-Black Border-yes")); + await waitFor(() => + expect(screen.getByTestId("attribute-chip-White Border")).toBeVisible() + ); + expect( + screen + .getByTestId("attribute-chip-Black Border") + .getAttribute("data-chip-state") + ).toBe("untouched"); + }); + it("reverts the explicit state on a failed submit", async () => { server.use( http.post(buildRoute("2/submitTagVote/"), () => diff --git a/frontend/src/features/attributeChips/AttributeChipPanel.tsx b/frontend/src/features/attributeChips/AttributeChipPanel.tsx index 130df8399..3a24b8792 100644 --- a/frontend/src/features/attributeChips/AttributeChipPanel.tsx +++ b/frontend/src/features/attributeChips/AttributeChipPanel.tsx @@ -27,8 +27,10 @@ import { } from "@/features/attributeChips/attributeChipRender"; import { ALL_ATTRIBUTE_CHIPS, + AttributeChipDef, ChipVoteState, EXCLUSION_GROUPS, + isChipContradicted, STANDALONE_CHIPS, } from "@/features/attributeChips/attributeChips"; import { useTagVoting } from "@/features/attributeChips/useTagVoting"; @@ -155,6 +157,15 @@ interface AttributeChipPanelProps { * but stays optional to match the same safe-default convention as the other funnel * components (see ArtistVotePicker.tsx's identical prop for the full rationale). */ onRateLimited?: () => void; + /** Context-dependent disqualification (DESIGN-REPASS-2026-08.md Rule 5): when true, an + * untouched chip whose own exclusion group already has an explicitly-positive sibling is + * hidden entirely rather than rendered dimmed (implied-negative). An active positive answer + * has already ruled the sibling values out - a card has one border color / one frame era - + * so the disqualified chips are dropped to reclaim their row space, mirroring how the deeper + * question grids prune options that contradict the answer given. False (default) keeps the + * historical dim-and-collapse treatment for callers that want the full taxonomy visible + * (the /display rail's AttributesSection). */ + pruneContradicted?: boolean; } export function AttributeChipPanel({ @@ -165,6 +176,7 @@ export function AttributeChipPanel({ onChipStatesChange, cardSlot, onRateLimited, + pruneContradicted = false, }: AttributeChipPanelProps) { const getTagDisplayName = useTagDisplayName(); const { confidence, submittingTagName, tap } = useTagVoting({ @@ -184,6 +196,14 @@ export function AttributeChipPanel({ getTagDisplayName, }; + // Context-dependent disqualification (pruneContradicted) filters the render list, not the + // vote state - a disqualified chip is hidden but stays "untouched" in chipStates, so it + // springs straight back the moment its group's positive answer is retracted. + const visibleChips = (chips: AttributeChipDef[]) => + pruneContradicted + ? chips.filter((chip) => !isChipContradicted(chip.tagName, chipStates)) + : chips; + // EXCLUSION_GROUPS[0] (Border Color) renders left, [1] (Frame Style) renders right - an // arbitrary but fixed assignment, not a semantic left/right meaning for either group. const [leftGroup, rightGroup] = EXCLUSION_GROUPS; @@ -199,7 +219,7 @@ export function AttributeChipPanel({ ); const topArea = ( - {STANDALONE_CHIPS.map((chip) => + {visibleChips(STANDALONE_CHIPS).map((chip) => renderAttributeChip(chipArgs, chip.tagName, chip.label) )} @@ -208,7 +228,7 @@ export function AttributeChipPanel({ {leftGroup.label} - {leftGroup.chips.map((chip) => + {visibleChips(leftGroup.chips).map((chip) => renderAttributeChip(chipArgs, chip.tagName, chip.label) )} @@ -218,7 +238,7 @@ export function AttributeChipPanel({ {rightGroup.label} - {rightGroup.chips.map((chip) => + {visibleChips(rightGroup.chips).map((chip) => renderAttributeChip(chipArgs, chip.tagName, chip.label) )} diff --git a/frontend/src/features/attributeChips/attributeChipRender.tsx b/frontend/src/features/attributeChips/attributeChipRender.tsx index 5fa043965..1b32bdccd 100644 --- a/frontend/src/features/attributeChips/attributeChipRender.tsx +++ b/frontend/src/features/attributeChips/attributeChipRender.tsx @@ -25,7 +25,7 @@ import React from "react"; import { ChipVoteState, - findExclusionGroup, + isChipContradicted, } from "@/features/attributeChips/attributeChips"; // Mobile funnel pass (thumb-native tap targets): measured at ~30px tall against the previous @@ -63,7 +63,7 @@ export const ChipGroup = styled.div<{ impliedNegative: boolean }>` export const ChipLabel = styled.span<{ fill: string; collapsed?: boolean }>` background-color: ${(props) => props.fill}; - padding: ${(props) => (props.collapsed ? "0" : "0.35rem 0.5rem")}; + padding: ${(props) => (props.collapsed ? "0" : "0.3rem 0.45rem")}; width: ${(props) => (props.collapsed ? "0" : "auto")}; display: ${(props) => (props.collapsed ? "none" : "inline-flex")}; align-items: center; @@ -90,10 +90,11 @@ export const ChipStateButton = styled.button<{ font-weight: ${(props) => (props.$active ? 700 : 400)}; min-height: 44px; min-width: 32px; - padding: 0.35rem 0.4rem; + padding: 0.3rem 0.3rem; display: inline-flex; align-items: center; justify-content: center; + touch-action: manipulation; &:disabled { opacity: 0.5; @@ -103,7 +104,7 @@ export const ChipStateButton = styled.button<{ export const ChipRow = styled.div` display: flex; flex-wrap: wrap; - gap: 0.4rem; + gap: 0.3rem; justify-content: center; `; @@ -157,15 +158,7 @@ export function renderAttributeChip( const explicitState = chipStates[tagName] ?? "untouched"; const isPositive = explicitState === "positive"; const isNegative = explicitState === "negative"; - const group = findExclusionGroup(tagName); - const impliedNegative = - explicitState === "untouched" && - group != null && - group.chips.some( - (sibling) => - sibling.tagName !== tagName && - (chipStates[sibling.tagName] ?? "untouched") === "positive" - ); + const impliedNegative = isChipContradicted(tagName, chipStates); const lean = leanTooltip(confidence[tagName] ?? 0); const disabled = submittingTagName != null; const setState = (desired: "positive" | "negative") => diff --git a/frontend/src/features/attributeChips/attributeChips.test.ts b/frontend/src/features/attributeChips/attributeChips.test.ts index 2a8387db9..73f79b0d2 100644 --- a/frontend/src/features/attributeChips/attributeChips.test.ts +++ b/frontend/src/features/attributeChips/attributeChips.test.ts @@ -9,6 +9,7 @@ import { findExclusionGroup, getAutoTagChips, getOpenExclusionGroups, + isChipContradicted, nextChipState, } from "./attributeChips"; @@ -126,3 +127,43 @@ describe("getOpenExclusionGroups", () => { expect(openGroups.map((group) => group.id)).toEqual(["borderColor"]); }); }); + +describe("isChipContradicted", () => { + it("is true for an untouched exclusion-group sibling of an explicit positive", () => { + expect( + isChipContradicted("White Border", { "Black Border": "positive" }) + ).toBe(true); + expect( + isChipContradicted("Silver Border", { "Black Border": "positive" }) + ).toBe(true); + }); + + it("is false for the chip that owns the positive vote itself", () => { + expect( + isChipContradicted("Black Border", { "Black Border": "positive" }) + ).toBe(false); + }); + + it("is false for an explicitly-voted sibling, even when a group-mate is positive", () => { + // an explicit negative is itself an active filter, not a disqualified option + expect( + isChipContradicted("White Border", { + "Black Border": "positive", + "White Border": "negative", + }) + ).toBe(false); + }); + + it("is false for a negative vote alone - it does not rule out any sibling value", () => { + expect( + isChipContradicted("White Border", { "Black Border": "negative" }) + ).toBe(false); + }); + + it("is false for standalone chips, which have no exclusion group", () => { + expect(isChipContradicted("Full Art", { "Full Art": "positive" })).toBe( + false + ); + expect(isChipContradicted("Full Art", {})).toBe(false); + }); +}); diff --git a/frontend/src/features/attributeChips/attributeChips.ts b/frontend/src/features/attributeChips/attributeChips.ts index 898bde04b..48e8ee09b 100644 --- a/frontend/src/features/attributeChips/attributeChips.ts +++ b/frontend/src/features/attributeChips/attributeChips.ts @@ -150,6 +150,37 @@ export function findExclusionGroup( ); } +/** + * True when a chip is contradicted by the current vote state: it is itself untouched AND an + * explicitly-positive sibling in its own exclusion group already answers the group's question + * (a card has exactly one border color / one frame era, so "Black Border" being yes leaves + * "White Border"/"Silver Border" factually disqualified). Standalone chips and explicitly- + * voted chips (positive or negative) are never contradicted - the latter are themselves the + * active filters that disqualify others. + * + * Surfaces may either dim such chips (implied-negative styling) or hide them entirely - + * the question feed's filter panel hides them (context-dependent disqualification, + * DESIGN-REPASS-2026-08.md Rule 5), mirroring how the deeper question grids drop any option + * that contradicts the answer already given, rather than only greying it out. + */ +export function isChipContradicted( + tagName: string, + chipStates: Record +): boolean { + if ((chipStates[tagName] ?? "untouched") !== "untouched") { + return false; + } + const group = findExclusionGroup(tagName); + if (group == null) { + return false; + } + return group.chips.some( + (sibling) => + sibling.tagName !== tagName && + (chipStates[sibling.tagName] ?? "untouched") === "positive" + ); +} + /** * Filters candidates against the current explicit chip vote states: a positive chip drops * any candidate that doesn't match it, a negative chip drops any candidate that does. Implied- diff --git a/frontend/src/features/printingTags/cardPanel.tsx b/frontend/src/features/printingTags/cardPanel.tsx index 5ee0921a1..23946bfa4 100644 --- a/frontend/src/features/printingTags/cardPanel.tsx +++ b/frontend/src/features/printingTags/cardPanel.tsx @@ -275,6 +275,10 @@ export const CandidateButton = styled.button` color: inherit; text-align: left; cursor: pointer; + /* DESIGN-REPASS Rule 2 (#715) - kills the mobile double-tap-zoom gesture that swallows a + fast single tap (the first tap starts a zoom, the second lands the click, reading as + "needs two taps"), so every tap on a candidate tile registers on the first press. */ + touch-action: manipulation; /* Issue #705 - same fix as SuggestedThumb (QuestionFeed.tsx): clip to the rounded tile at rest, stop clipping for exactly the hover duration ZoomableThumbnail scales its up, diff --git a/frontend/src/features/questionFeed/QuestionFeed.test.tsx b/frontend/src/features/questionFeed/QuestionFeed.test.tsx index 94a6be7fb..94c3f8db8 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.test.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.test.tsx @@ -134,6 +134,42 @@ describe("QuestionFeed", () => { ).not.toBeInTheDocument(); }); + it("the feed's filter panel hides exclusion-group siblings of an explicit positive (context-dependent disqualification)", async () => { + server.use( + questionFeedOnce(), + http.post(buildRoute("2/submitTagVote/"), async ({ request }) => { + const body = (await request.json()) as { + tagName: string; + polarity: number; + }; + return HttpResponse.json( + { + tagName: body.tagName, + resolvedPolarity: null, + netPolarity: body.polarity, + tally: [], + }, + { status: 200 } + ); + }) + ); + renderFeed(); + await revealCard(); + await screen.findByTestId("attribute-chip-Black Border"); + + fireEvent.click(screen.getByTestId("attribute-chip-Black Border-yes")); + await waitFor(() => + expect( + screen + .getByTestId("attribute-chip-Black Border") + .getAttribute("data-chip-state") + ).toBe("positive") + ); + // the contradicted siblings are pruned from the feed's panel, not just dimmed + expect(screen.queryByTestId("attribute-chip-White Border")).toBeNull(); + expect(screen.queryByTestId("attribute-chip-Silver Border")).toBeNull(); + }); + it("clicking 'None of these' submits a no-match printing vote", async () => { server.use(questionFeedOnce()); let submittedIsNoMatch: boolean | undefined; diff --git a/frontend/src/features/questionFeed/QuestionFeed.tsx b/frontend/src/features/questionFeed/QuestionFeed.tsx index 2280248bb..84ec9b72d 100644 --- a/frontend/src/features/questionFeed/QuestionFeed.tsx +++ b/frontend/src/features/questionFeed/QuestionFeed.tsx @@ -81,6 +81,7 @@ import { CandidateGrid, CARD_ASPECT_RATIO, CardPanel, + ILLUSTRATION_CROP_ASPECT_RATIO, IllustrationArtPlaceholder, MysteryCard, randomFlavorText, @@ -236,9 +237,15 @@ const SubjectArt = styled.div` } `; -const SubjectArtImage = styled.div` +// DESIGN-REPASS Rule 3 (#746) - `$landscape` is set only by the artist-question subject slot, +// whose image is the harvested Scryfall illustration crop (a landscape 584/444 region, not a +// card scan). Rendering it in the portrait card frame cropped its top and bottom off inside a +// box sized for a card; the landscape frame is the same ratio the illustration-group candidate +// tiles already use, so the same artwork reads at the same proportions wherever it appears. +const SubjectArtImage = styled.div<{ $landscape?: boolean }>` position: relative; - aspect-ratio: ${CARD_ASPECT_RATIO}; + aspect-ratio: ${(props) => + props.$landscape ? ILLUSTRATION_CROP_ASPECT_RATIO : CARD_ASPECT_RATIO}; @container hero (max-width: 560px) { flex: 1; @@ -365,8 +372,17 @@ const IllustrationGroupLabel = styled.p` // Caps the width of the reused ArtistSupportLink applet so a full-bleed button (its own // "stretch to fill" rule - see the component's docstring, not overridden here) reads as a // compact cluster credit rather than a page-width CTA repeated once per cluster. -const IllustrationCredit = styled.div` +// DESIGN-REPASS Rule 6 (#711-adjacent MTGAC work) - the ArtistSupportLink applet renders in two +// places on this surface (under an illustration cluster's credit, and under the artist +// question's post-answer moment) and must look the same in both. The illustration credit +// always capped it at 220px; the post-answer banner had no cap and let the collapsed row's +// primary link stretch to the full question-panel width. One shared cap makes the two renders +// identical instead of one tidy 220px line and one full-width button. +const ArtistCredit = styled.div` max-width: 220px; +`; + +const IllustrationCredit = styled(ArtistCredit)` margin-bottom: 8px; `; @@ -402,18 +418,15 @@ const Btn = styled.button` cursor: pointer; line-height: 1.2; text-align: center; + /* DESIGN-REPASS Rule 2 (#715) - opt out of the mobile double-tap-zoom gesture, which + otherwise swallows a fast single tap and reads as "this button needs two taps". */ + touch-action: manipulation; &:disabled { opacity: 0.6; cursor: default; } - &.big { - font-size: 17px; - font-weight: 800; - padding: 10px 20px; - } - &.block { width: 100%; } @@ -642,6 +655,7 @@ const TriStateChip = styled.button` color: var(--text); border: 1px solid var(--muted); border-radius: var(--r-btn); + touch-action: manipulation; &:disabled { opacity: 0.6; @@ -724,6 +738,16 @@ export function QuestionFeed() { const [imageGeneration, setImageGeneration] = useState(0); const cardImageRef = useRef(null); const [submitting, setSubmitting] = useState(false); + // DESIGN-REPASS Rule 2 (#715) - `disabled={submitting}` only takes effect on the re-render + // React batches AFTER the current event handler returns, so two taps inside that window (a + // double-click, or a stray second tap on a tile) both re-enter the vote handler and cast the + // vote twice. This ref is set synchronously at handler entry and read at the top of every + // handler, closing the window - the second entry is dropped, not queued - while the state + // flag keeps driving the disabled/visual state. Advance-only handlers (skip, Not sure) hold + // it until the next item lands (the fetch effect clears it) so a double-tap can't skip two + // cards; vote handlers release it in their own `.finally` (a Level 3 transition must re-enable + // the chips immediately). + const voteInFlightRef = useRef(false); const [selectedCandidateId, setSelectedCandidateId] = useState( null ); @@ -816,6 +840,9 @@ export function QuestionFeed() { setImageLoaded(false); setImageErrored(false); setImageGeneration((previous) => previous + 1); + // A new item has landed (or the feed is empty) - release any advance-only in-flight + // ref (skip / Not sure) so the next question's controls are live immediately. + voteInFlightRef.current = false; // A genuinely empty configured URL (this test suite's own fixture convention - real // cards always carry a real CDN URL) has nothing to load at all, so it's settled right // here rather than waiting on any image event. @@ -844,6 +871,7 @@ export function QuestionFeed() { setLevel3Active(false); }) .catch(() => { + voteInFlightRef.current = false; setItem(null); setFetchError(true); }) @@ -914,9 +942,10 @@ export function QuestionFeed() { candidate: PrintingCandidate | undefined, isNoMatch: boolean ) => { - if (backendURL == null || item == null) { + if (backendURL == null || item == null || voteInFlightRef.current) { return; } + voteInFlightRef.current = true; setSubmitting(true); setSelectedCandidateId(candidate?.identifier ?? "no-match"); const anonymousId = getOrCreateAnonymousId(); @@ -969,6 +998,7 @@ export function QuestionFeed() { }) .catch(reportVoteFailed) .finally(() => { + voteInFlightRef.current = false; setSubmitting(false); setSelectedCandidateId(null); }); @@ -989,9 +1019,10 @@ export function QuestionFeed() { illustrationId: string, tappedCandidate: PrintingCandidate ) => { - if (backendURL == null || item == null) { + if (backendURL == null || item == null || voteInFlightRef.current) { return; } + voteInFlightRef.current = true; setSubmitting(true); setSelectedCandidateId(tappedCandidate.identifier); const anonymousId = getOrCreateAnonymousId(); @@ -1010,6 +1041,7 @@ export function QuestionFeed() { }) .catch(reportVoteFailed) .finally(() => { + voteInFlightRef.current = false; setSubmitting(false); setSelectedCandidateId(null); }); @@ -1019,9 +1051,10 @@ export function QuestionFeed() { // instead of "None of these" -> the reason strip, since the tap already told us why (see // reason_tags.py's existing seeded "custom-art" tag - no new endpoint). const classifyAsCustomArt = () => { - if (backendURL == null || item == null) { + if (backendURL == null || item == null || voteInFlightRef.current) { return; } + voteInFlightRef.current = true; setSubmitting(true); setSelectedCandidateId("custom-art"); const anonymousId = getOrCreateAnonymousId(); @@ -1048,6 +1081,7 @@ export function QuestionFeed() { }) .catch(reportVoteFailed) .finally(() => { + voteInFlightRef.current = false; setSubmitting(false); setSelectedCandidateId(null); }); @@ -1069,6 +1103,9 @@ export function QuestionFeed() { }; const confirmLevel3 = () => { + if (voteInFlightRef.current) { + return; + } if (backendURL == null || item == null) { advance(); return; @@ -1081,6 +1118,7 @@ export function QuestionFeed() { advance(); return; } + voteInFlightRef.current = true; setSubmitting(true); Promise.all( picked.map(([tagName]) => @@ -1100,10 +1138,21 @@ export function QuestionFeed() { advance(); }) .catch(reportVoteFailed) - .finally(() => setSubmitting(false)); + .finally(() => { + voteInFlightRef.current = false; + setSubmitting(false); + }); }; - const skip = () => advance(); + // Advance-only handlers hold the in-flight ref until the next item lands (the fetch effect + // clears it) - a double-tap on Skip / Not sure must not advance two cards at once. + const skip = () => { + if (voteInFlightRef.current) { + return; + } + voteInFlightRef.current = true; + advance(); + }; // Records the "Not sure" abstention (issue #712) and moves on - fire-and-forget, same // best-effort convention as the auto-tag-chip casts in selectCandidate above: the write is @@ -1111,6 +1160,10 @@ export function QuestionFeed() { // ladder to fall into (issue #728): "Not sure" means "I can't resolve this", so it advances // to the next question rather than re-asking the same candidates on another page. const submitNotSure = () => { + if (voteInFlightRef.current) { + return; + } + voteInFlightRef.current = true; if (backendURL != null && item != null) { APISubmitQuestionAbstention( backendURL, @@ -1635,6 +1688,7 @@ export function QuestionFeed() { chipStates={chipStates} onChipStatesChange={setChipStates} onRateLimited={() => setRateLimited(true)} + pruneContradicted /> )} @@ -1808,7 +1862,10 @@ export function QuestionFeed() { cardNode = ( - + {confirmedArtistName != null && ( -
+ -
+ )}