Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions .github/coverage-acks.txt
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,13 @@ coverage-ack: frontend/tests/DisplayPage.spec.ts::DisplayPage (Proposal H, Step
# Same surface, same assertions, only the URL (and the describe/test title naming it) changed.
coverage-ack: frontend/tests/ContrastAudit.spec.ts::Contrast audit - /contributions (owner defect 1+2: accordion header + body) > Contribution Guidelines accordion - collapsed and expanded — route moved /contributions -> /stats in PR #558; same surface, same assertions
coverage-ack: frontend/tests/ContrastAudit.spec.ts::Link colour audit (owner-approved open item 2 - $link-color -> $theme-info) > a real production link (contributions guidelines' ISO-639-1 reference, inside the accordion body's panel-bg) clears strict-AAA-normal — route moved /contributions -> /stats in PR #558; same surface, same assertions

# PR #730 - QuestionFeed.tsx's illustration-group render now emits ONE representative tile per
# group (`group.find((c) => c.artCropUrl) ?? group[0]`) instead of one tile per member, so a
# non-representative member (candidateB in this fixture set) no longer gets its own DOM tile
# anywhere - it's represented by the group's shared vote, not silently dropped. The three old
# titles asserted the retired one-tile-per-member behavior; all three are renamed (not deleted)
# to describe the new representative-tile contract, same surface, same fixture set.
coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > every candidate in a mixed illustration set still renders in the grid - none silently dropped — PR #730: renamed to "every candidate in a mixed illustration set is accounted for - a group collapses to its one representative tile, ungrouped candidates still render individually" now that a group renders one representative tile, not one tile per member.
coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > candidates sharing an illustration render inside one illustration-group container; unique/null-illustration candidates don't — PR #730: renamed to "only the illustration group's representative candidate renders inside its one illustration-group container; unique/null-illustration candidates don't", same reason as the sibling ack above.
coverage-ack: frontend/tests/QuestionFeed.spec.ts::question feed - Level 2 illustration grouping > clustered tiles render each candidate's art crop, falling back to the printing scan when absent; ungrouped tiles keep the printing scan regardless — PR #730: renamed to "the illustration group's one tile renders its representative candidate's art crop; ungrouped tiles keep the printing scan regardless", same reason as the sibling acks above.
93 changes: 88 additions & 5 deletions frontend/src/features/questionFeed/QuestionFeed.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ describe("QuestionFeed", () => {
expect(illustrationVoteCalled).toBe(false);
});

it("clustered tiles render the candidate's art crop, falling back to the printing scan when absent; ungrouped tiles are unaffected", async () => {
it("renders exactly one representative tile per illustration group, preferring a member with an art crop; ungrouped tiles are unaffected", async () => {
const withArtCrop = {
...groupedItem,
candidates: [
Expand Down Expand Up @@ -930,10 +930,10 @@ describe("QuestionFeed", () => {
"src",
"https://example.com/art-crop-1.png"
);
expect(within(group).getByAltText("xyz 42")).toHaveAttribute(
"src",
groupedItem.candidates[1].mediumThumbnailUrl
);
expect(within(group).queryByAltText("xyz 42")).not.toBeInTheDocument();
expect(
within(group).getByText("Same illustration - 2 printings")
).toBeInTheDocument();

const ungroupedGrid = await screen.findByTestId(
"question-feed-candidate-grid-ungrouped"
Expand All @@ -943,5 +943,88 @@ describe("QuestionFeed", () => {
groupedItem.candidates[2].mediumThumbnailUrl
);
});

it("falls back to the group's first member when no member has an art crop", async () => {
server.use(groupedQuestionFeedOnce());
renderFeed();
await revealCard();

const group = await screen.findByTestId(
"question-feed-illustration-group"
);
expect(within(group).getByAltText("abc 1")).toHaveAttribute(
"src",
groupedItem.candidates[0].mediumThumbnailUrl
);
expect(within(group).queryByAltText("xyz 42")).not.toBeInTheDocument();
});

it("picks whichever group member has an art crop regardless of position, and still submits the group's shared illustrationId", async () => {
const artCropOnSecondMember = {
...groupedItem,
candidates: [
{ ...groupedItem.candidates[0], artCropUrl: null },
{
...groupedItem.candidates[1],
artCropUrl: "https://example.com/art-crop-2.png",
},
groupedItem.candidates[2],
],
};
server.use(
http.get(buildRoute("2/questionFeed/"), () =>
HttpResponse.json(
{
item: artCropOnSecondMember,
remainingEstimate: {
total: 1,
confirmable: 0,
contested: 0,
fresh: 1,
},
},
{ status: 200 }
)
)
);
let illustrationVoteBody: Record<string, unknown> | undefined;
server.use(
http.post(
buildRoute("2/submitIllustrationVote/"),
async ({ request }) => {
illustrationVoteBody = (await request.json()) as Record<
string,
unknown
>;
return HttpResponse.json(
{
illustrationId: sharedIllustrationId,
isUnknown: false,
printingVoteCast: false,
artistVoteCast: true,
},
{ status: 200 }
);
}
)
);
renderFeed();
await revealCard();

const group = await screen.findByTestId(
"question-feed-illustration-group"
);
const tile = within(group).getByAltText("xyz 42");
expect(tile).toHaveAttribute("src", "https://example.com/art-crop-2.png");
expect(within(group).queryByAltText("abc 1")).not.toBeInTheDocument();

fireEvent.click(tile);

await waitFor(() => expect(illustrationVoteBody).toBeDefined());
expect(illustrationVoteBody).toMatchObject({
identifier: groupedItem.card.identifier,
illustrationId: sharedIllustrationId,
});
});
});
});
35 changes: 22 additions & 13 deletions frontend/src/features/questionFeed/QuestionFeed.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1619,6 +1619,14 @@ export function QuestionFeed() {
const illustrationArtist = group
.map((candidate) => candidate.artist)
.find((artist) => artist.trim() !== "");
// selectIllustrationGroup already submits one illustrationId for the whole
// cluster (see that function's own comment), so one tile fully represents what
// is being voted on - showing every member here just repeats the same artwork
// up to N times. Prefer a member with an art crop, the same signal a tile's own
// image already prefers (renderCandidateTile's imageUrl default below), and fall
// back to the first member so the same data always picks the same tile.
const representative =
group.find((candidate) => candidate.artCropUrl) ?? group[0];
return (
<IllustrationGroup
key={group[0].illustrationId}
Expand All @@ -1637,19 +1645,20 @@ export function QuestionFeed() {
</IllustrationCredit>
)}
<CandidateGrid>
{group.map((candidate) =>
renderCandidateTile(
candidate,
() =>
// every member of `group` shares this non-null illustrationId - see the
// grouping logic above, which only clusters candidates that have one.
selectIllustrationGroup(
candidate.illustrationId as string,
candidate
),
false,
candidate.artCropUrl || candidate.mediumThumbnailUrl
)
{renderCandidateTile(
representative,
() =>
// every member of `group` shares this non-null illustrationId - see
// the grouping logic above, which only clusters candidates that have
// one - so submitting the representative's illustrationId is
// identical to submitting any other member's.
selectIllustrationGroup(
representative.illustrationId as string,
representative
),
false,
representative.artCropUrl ||
representative.mediumThumbnailUrl
)}
</CandidateGrid>
</IllustrationGroup>
Expand Down
33 changes: 22 additions & 11 deletions frontend/tests/QuestionFeed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ test.describe("question feed - Level 3 (conditional open-attribute confirm)", ()
// frequently-absent shape (CanonicalPrintingMetadata.illustration_id, see
// local_illustration.py:137's isnull filter).
test.describe("question feed - Level 2 illustration grouping", () => {
test("every candidate in a mixed illustration set still renders in the grid - none silently dropped", async ({
test("every candidate in a mixed illustration set is accounted for - a group collapses to its one representative tile, ungrouped candidates still render individually", async ({
page,
network,
}) => {
Expand All @@ -242,12 +242,26 @@ test.describe("question feed - Level 2 illustration grouping", () => {
);
await loadPageWithDefaultBackend(page, "whatsthat");

// candidateA carries the art crop, so it's the group's chosen representative
// (QuestionFeed.tsx's `group.find((c) => c.artCropUrl) ?? group[0]`) - the group renders
// ONE tile for the whole cluster, not one per member.
await expect(
page.locator(
`[data-card-identifier="${illustrationGroupCandidateA.identifier}"]`
)
).toHaveCount(1);
// candidateB shares the illustration but loses the representative pick to A - it renders
// no tile of its own anywhere; its vote is carried by A's tile, not silently dropped.
await expect(
page.locator(
`[data-card-identifier="${illustrationGroupCandidateB.identifier}"]`
)
).toHaveCount(0);

// The regression guard this task calls out explicitly: an exact count, not just "at least
// one" - a candidate silently vanishing (e.g. because it has no illustrationId) is exactly
// the correctness regression a weaker assertion would miss.
for (const candidate of [
illustrationGroupCandidateA,
illustrationGroupCandidateB,
illustrationGroupCandidateC,
illustrationGroupCandidateD,
]) {
Expand All @@ -257,7 +271,7 @@ test.describe("question feed - Level 2 illustration grouping", () => {
}
});

test("candidates sharing an illustration render inside one illustration-group container; unique/null-illustration candidates don't", async ({
test("only the illustration group's representative candidate renders inside its one illustration-group container; unique/null-illustration candidates don't", async ({
page,
network,
}) => {
Expand All @@ -278,11 +292,13 @@ test.describe("question feed - Level 2 illustration grouping", () => {
`[data-card-identifier="${illustrationGroupCandidateA.identifier}"]`
)
).toHaveCount(1);
// candidateB shares the illustration but has no art crop, so A wins the representative
// pick and the group renders only A's tile - not B's.
await expect(
group.locator(
`[data-card-identifier="${illustrationGroupCandidateB.identifier}"]`
)
).toHaveCount(1);
).toHaveCount(0);

// candidateC (distinct illustrationId, no sibling) and candidateD (null illustrationId)
// both render OUTSIDE the clustered group - neither forms (or joins) a cluster of one.
Expand All @@ -298,7 +314,7 @@ test.describe("question feed - Level 2 illustration grouping", () => {
).toHaveCount(0);
});

test("clustered tiles render each candidate's art crop, falling back to the printing scan when absent; ungrouped tiles keep the printing scan regardless", async ({
test("the illustration group's one tile renders its representative candidate's art crop; ungrouped tiles keep the printing scan regardless", async ({
page,
network,
}) => {
Expand All @@ -314,11 +330,6 @@ test.describe("question feed - Level 2 illustration grouping", () => {
`[data-card-identifier="${illustrationGroupCandidateA.identifier}"] img`
)
).toHaveAttribute("src", illustrationGroupCandidateA.artCropUrl as string);
await expect(
group.locator(
`[data-card-identifier="${illustrationGroupCandidateB.identifier}"] img`
)
).toHaveAttribute("src", illustrationGroupCandidateB.mediumThumbnailUrl);

const ungroupedGrid = page.getByTestId(
"question-feed-candidate-grid-ungrouped"
Expand Down