diff --git a/frontend/src/components/ArtistSupportLink.test.tsx b/frontend/src/components/ArtistSupportLink.test.tsx index a5f4c83a2..6e68a113f 100644 --- a/frontend/src/components/ArtistSupportLink.test.tsx +++ b/frontend/src/components/ArtistSupportLink.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, waitFor } from "@testing-library/react"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import React from "react"; import { Provider } from "react-redux"; @@ -18,15 +18,28 @@ import { setupStore } from "@/store/store"; import { ArtistSupportLink, buildArtistSupportURL } from "./ArtistSupportLink"; -function renderApplet(artistName: string, backend = localBackend) { +function renderApplet( + artistName: string, + backend = localBackend, + defaultExpanded = false +) { const store = setupStore({ backend }); render( - + ); } +// Issue #709 - the applet defaults to collapsed (a single line: the page-link + a disclosure +// toggle); this expands it so tests can assert on the commerce links/badge/credit it reveals. +function expandApplet() { + fireEvent.click(screen.getByTestId("artist-support-toggle")); +} + describe("buildArtistSupportURL", () => { it("URL-encodes the artist name into an MTG Artist Connection artist-page URL", () => { expect(buildArtistSupportURL("Harold McNeill")).toBe( @@ -42,7 +55,7 @@ describe("buildArtistSupportURL", () => { }); describe("ArtistSupportLink applet", () => { - it("never renders as an empty box: the MTGAC page link and the credit are present before any network response resolves", () => { + it("never renders as an empty box: the MTGAC page link is present before any network response resolves, collapsed to a single line by default", () => { // no server.use(...) at all - the request is in flight (or, with noBackend below, never // even fires) - the applet's base shape must already be there, not a spinner/empty state. renderApplet("Harold McNeill"); @@ -52,9 +65,10 @@ describe("ArtistSupportLink applet", () => { "href", buildArtistSupportURL("Harold McNeill") ); - expect(screen.getByTestId("artist-support-credit")).toHaveTextContent( - "MTG Artist Connection" - ); + expect(screen.getByTestId("artist-support-toggle")).toBeInTheDocument(); + expect( + screen.queryByTestId("artist-support-credit") + ).not.toBeInTheDocument(); expect( screen.queryByTestId("artist-support-commerce-links") ).not.toBeInTheDocument(); @@ -63,7 +77,36 @@ describe("ArtistSupportLink applet", () => { ).not.toBeInTheDocument(); }); - it("with no remote backend configured, still renders the fallback applet and makes no request at all", () => { + it("expanding the collapsed applet reveals the credit line, even with nothing else to show", () => { + renderApplet("Harold McNeill"); + + expandApplet(); + + expect(screen.getByTestId("artist-support-credit")).toHaveTextContent( + "MTG Artist Connection" + ); + }); + + it("collapsing again after expanding hides the credit line without unmounting the applet", () => { + renderApplet("Harold McNeill"); + + expandApplet(); + expect(screen.getByTestId("artist-support-credit")).toBeInTheDocument(); + + expandApplet(); + expect( + screen.queryByTestId("artist-support-credit") + ).not.toBeInTheDocument(); + expect(screen.getByTestId("artist-support-applet")).toBeInTheDocument(); + }); + + it("defaultExpanded starts the applet open, with the credit line visible without any interaction (the /editor rail's usage)", () => { + renderApplet("Harold McNeill", localBackend, true); + + expect(screen.getByTestId("artist-support-credit")).toBeInTheDocument(); + }); + + it("with no remote backend configured, still renders the fallback link and makes no request at all", () => { // No server.use(...) either - if a request were made, MSW's onUnhandledRequest: "error" // config would fail this test, so a clean pass here IS the "no request" assertion. renderApplet("Harold McNeill", noBackend); @@ -72,6 +115,8 @@ describe("ArtistSupportLink applet", () => { "href", buildArtistSupportURL("Harold McNeill") ); + + expandApplet(); expect(screen.getByTestId("artist-support-credit")).toBeInTheDocument(); }); @@ -85,6 +130,7 @@ describe("ArtistSupportLink applet", () => { buildArtistSupportURL("Harold McNeill") ) ); + expandApplet(); expect( screen.queryByTestId("artist-support-commerce-links") ).not.toBeInTheDocument(); @@ -94,7 +140,7 @@ describe("ArtistSupportLink applet", () => { expect(screen.getByTestId("artist-support-credit")).toBeInTheDocument(); }); - it("zero commerce links (found: true): still renders the MTGAC page link and the credit, no commerce buttons, no empty box", async () => { + it("zero commerce links (found: true): still renders the MTGAC page link and (once expanded) the credit, no commerce buttons, no empty box", async () => { server.use(artistExternalLinksZeroLinks); renderApplet(ArtistExternalLinksTestArtists.zeroLinks); @@ -106,6 +152,7 @@ describe("ArtistSupportLink applet", () => { )}` ) ); + expandApplet(); expect( screen.queryByTestId("artist-support-commerce-links") ).not.toBeInTheDocument(); @@ -137,10 +184,15 @@ describe("ArtistSupportLink applet", () => { ); }); - it("exactly one commerce link renders one stretched button", async () => { + it("exactly one commerce link renders one stretched button, reachable via the expand toggle", async () => { server.use(artistExternalLinksOneLink); renderApplet(ArtistExternalLinksTestArtists.oneLink); + await waitFor(() => + expect(screen.getByTestId("artist-support-toggle")).toBeInTheDocument() + ); + expandApplet(); + const links = await screen.findAllByTestId("artist-support-commerce-link"); expect(links).toHaveLength(1); expect(links[0]).toHaveAttribute( @@ -153,6 +205,7 @@ describe("ArtistSupportLink applet", () => { it("full row (5 commerce links) renders in the fixed priority order, capped at 5", async () => { server.use(artistExternalLinksFullRow); renderApplet(ArtistExternalLinksTestArtists.fullRow); + expandApplet(); const links = await screen.findAllByTestId("artist-support-commerce-link"); expect(links).toHaveLength(5); @@ -168,6 +221,7 @@ describe("ArtistSupportLink applet", () => { it("an artist whose only link is instagram surfaces it (the 157-artist rescue scenario)", async () => { server.use(artistExternalLinksInstagramOnly); renderApplet(ArtistExternalLinksTestArtists.instagramOnly); + expandApplet(); const links = await screen.findAllByTestId("artist-support-commerce-link"); expect(links).toHaveLength(1); @@ -178,6 +232,7 @@ describe("ArtistSupportLink applet", () => { it("the Mark's Signature Service flag renders as a badge, never as a link", async () => { server.use(artistExternalLinksWithSignatureBadge); renderApplet(ArtistExternalLinksTestArtists.signatureService); + expandApplet(); const badge = await screen.findByTestId("artist-support-signature-badge"); expect(badge.tagName).toBe("SPAN"); @@ -193,9 +248,10 @@ describe("ArtistSupportLink applet", () => { } }); - it("the MTG Artist Connection credit is always present and links to their homepage", async () => { + it("the MTG Artist Connection credit is always reachable and links to their homepage", async () => { server.use(artistExternalLinksOneLink); renderApplet(ArtistExternalLinksTestArtists.oneLink); + expandApplet(); const credit = await screen.findByTestId("artist-support-credit"); expect(credit).toHaveTextContent("MTG Artist Connection"); @@ -214,6 +270,7 @@ describe("ArtistSupportLink applet", () => { expect(primaryLink).toHaveAttribute("target", "_blank"); expect(primaryLink).toHaveAttribute("rel", "noopener noreferrer"); + expandApplet(); const commerceLink = await screen.findByTestId( "artist-support-commerce-link" ); diff --git a/frontend/src/components/ArtistSupportLink.tsx b/frontend/src/components/ArtistSupportLink.tsx index c0afff16c..5e9d8704f 100644 --- a/frontend/src/components/ArtistSupportLink.tsx +++ b/frontend/src/components/ArtistSupportLink.tsx @@ -13,33 +13,30 @@ * still-loading request - are treated identically here: fall back to the deterministic * `buildArtistSupportURL` construction. **Never a broken or empty state.** * + * **Compact by default, expandable** (issue #709 - the applet used to always stack the page-link + * button, up to five commerce buttons, a badge, and a credit line, up to ~8 rows deep next to + * whatever the surface was actually about). The default render is ONE line: the artist page link + * plus a small disclosure toggle. Commerce links, the signature badge, and the MTGAC credit line + * move into a panel that only mounts once the user opts in - every link that existed before is + * still reachable, just not always paid for in vertical space. + * * **Design target: zero and one commerce links, not four or five.** Measured against the real * 2,389-artist export: 812 have zero commerce links, 818 have exactly one, only 13 have all five * (instagram - added later as a last-resort exception, not a commerce field - rescues 157 of * those 812 down to 655; ~598 have nothing but the MTGAC page link regardless of what's - * allowlisted). This applet ALWAYS renders the MTGAC page link and the "Source: MTG Artist - * Connection" credit (an obligation, not decoration - site credits were part of the MTGAC - * partnership) - commerce buttons are purely additive on top of that stable base, so the ~69% of - * artists with zero or one link see a small, correctly-proportioned applet, not an empty box or - * a layout built for a row of five buttons that's usually mostly missing. - * - * **No layout shift while loading**: the loading state and the "found: false"/zero-commerce-link - * state render IDENTICALLY (MTGAC page link + credit only, no commerce buttons, no badge). - * Commerce buttons and the signature-service badge are added on top once data confirms they - * exist - for a third of all artists that's not a shift at all, since the loaded state IS the - * base shape; for the rest, buttons appear below the stable base rather than a spinner/skeleton - * collapsing into a completely different-shaped final layout. - * - * **Buttons stretch to fill the applet** (owner instruction) - every rendered ``/button here - * is full-width within whatever container the caller gives it, so the applet reads consistently - * whether that container is a narrow table cell or a full-width rail panel. + * allowlisted). The "Source: MTG Artist Connection" credit (an obligation, not decoration - site + * credits were part of the MTGAC partnership) is always reachable through the disclosure, whether + * or not this particular artist has any commerce links to go with it. * * Callers gate rendering on the artist being confirmed/known (the same precedence chain * `Card.serialise` exposes via `canonicalArtist`, or a vote just cast) - this component has no * opinion on that and never widens it; it only takes `artistName` (required, non-nullable) plus - * an optional `className` for the caller's own layout/spacing. + * an optional `className` for the caller's own layout/spacing, and an optional `defaultExpanded` + * for the one surface (the /editor rail's Artist section) that has room to show the full applet + * up front. */ -import React from "react"; +import styled from "@emotion/styled"; +import React, { useId, useState } from "react"; import { MTGArtistConnection, @@ -69,16 +66,68 @@ const LINK_TYPE_LABELS: Record = { instagram: "Instagram", }; +// The collapsed row - primary link + toggle - never wraps to a second line, regardless of how +// narrow the caller's container is (the question feed's illustration credit caps it at 220px). +const CompactLine = styled.div` + display: flex; + align-items: center; + gap: 6px; + min-width: 0; +`; + +const CompactLink = styled.a` + display: inline-flex; + align-items: center; + gap: 4px; + min-width: 0; + flex: 1 1 auto; +`; + +// The artist name is the part that can be arbitrarily long, so ellipsis lives on this inner +// span rather than the flex anchor itself (text-overflow needs a single-line inline box, not a +// flex container with a sibling icon). +const CompactLinkLabel = styled.span` + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + min-width: 0; +`; + +const ExpandToggle = styled.button` + flex: 0 0 auto; + display: inline-flex; + align-items: center; + justify-content: center; + width: 28px; + height: 28px; + padding: 0; + background: transparent; + border: 1px solid var(--bs-border-color, currentColor); + border-radius: var(--r-btn, 4px); + color: inherit; + cursor: pointer; +`; + interface ArtistSupportLinkProps { artistName: string; className?: string; + /** The /editor rail's Artist section (ArtistSection.tsx) is a dedicated accordion pane with + * room for the full stacked applet, and its own Playwright coverage already asserts the credit + * line renders without any interaction - so that one caller opts into starting expanded. Every + * other caller (the question feed's inline credits, the card-detail modal's metadata table) is + * space-constrained next to whatever the surface is actually about, so `undefined`/`false` + * (the default) starts collapsed to a single line. */ + defaultExpanded?: boolean; } export function ArtistSupportLink({ artistName, className, + defaultExpanded = false, }: ArtistSupportLinkProps) { const { data } = useGetArtistExternalLinksQuery(artistName); + const [expanded, setExpanded] = useState(defaultExpanded); + const expandedPanelId = useId(); // Prefer MTGAC's own authoritative pageUrl when we have it - this is the actual point of the // applet, not a nice-to-have: 197/2,389 (8.2%) of this project's deterministically-constructed @@ -99,58 +148,77 @@ export function ArtistSupportLink({ className={["artist-support-applet", className].filter(Boolean).join(" ")} data-testid="artist-support-applet" > - - {artistName} - - {commerceLinks.length > 0 && ( -
+ - {commerceLinks.map((link) => ( + {artistName} + + + setExpanded((previous) => !previous)} + aria-expanded={expanded} + aria-controls={expandedPanelId} + data-testid="artist-support-toggle" + > + + + {expanded ? "Hide artist links" : "Show more artist links"} + + + + {expanded && ( +
+ {commerceLinks.length > 0 && ( +
+ {commerceLinks.map((link) => ( + + {LINK_TYPE_LABELS[link.type] ?? link.type}{" "} + + + ))} +
+ )} + {hasSignatureService && ( + + Mark's Signature Service + + )} +
+ Source:{" "} - {LINK_TYPE_LABELS[link.type] ?? link.type}{" "} - + {MTGArtistConnection} - ))} +
)} - {hasSignatureService && ( - - Mark's Signature Service - - )} -
- Source:{" "} - - {MTGArtistConnection} - -
); } diff --git a/frontend/src/features/display/ArtistSection.tsx b/frontend/src/features/display/ArtistSection.tsx index c85324ab2..e6cdca362 100644 --- a/frontend/src/features/display/ArtistSection.tsx +++ b/frontend/src/features/display/ArtistSection.tsx @@ -46,7 +46,10 @@ export function ArtistSection({ cardDocument }: ArtistSectionProps) { {cardDocument.canonicalArtist.name} - + ) : ( Unknown diff --git a/frontend/tests/QuestionFeed.spec.ts b/frontend/tests/QuestionFeed.spec.ts index 2012da6cf..edf7b50f6 100644 --- a/frontend/tests/QuestionFeed.spec.ts +++ b/frontend/tests/QuestionFeed.spec.ts @@ -341,6 +341,62 @@ test.describe("question feed - Level 2 illustration grouping", () => { ).toHaveAttribute("src", illustrationGroupCandidateC.mediumThumbnailUrl); }); + // Issue #709 - the illustration-credit ArtistSupportLink used to always stack the page-link + // button, up to five commerce buttons, a badge, and a credit line next to the question - up to + // ~8 rows. It now defaults to one collapsed line and expands on demand; the expansion must + // never cover the pinned reference image (SPEC-wtc-rebuild.md Amendment A2). + test("the illustration-credit Artist Support Link is compact by default and its expansion never overlaps the pinned reference image", async ({ + page, + network, + }) => { + network.use( + questionFeedIdentifyPrintingGroupedByIllustration, + ...defaultHandlers + ); + await loadPageWithDefaultBackend(page, "whatsthat"); + + const credit = page.getByTestId("question-feed-illustration-credit"); + const applet = credit.getByTestId("artist-support-applet"); + await expect(applet.getByTestId("artist-support-link")).toContainText( + "Some Artist" + ); + await expect(applet.getByTestId("artist-support-credit")).toHaveCount(0); + await expect( + applet.getByTestId("artist-support-commerce-links") + ).toHaveCount(0); + + // Collapsed: one line, well under the height a stacked applet (page link + credit, let + // alone commerce buttons) would need. + const collapsedBox = await applet.boundingBox(); + expect(collapsedBox).not.toBeNull(); + expect((collapsedBox as { height: number }).height).toBeLessThan(40); + + await applet.getByTestId("artist-support-toggle").click(); + await expect(applet.getByTestId("artist-support-credit")).toBeVisible(); + + const subjectBox = await page + .getByTestId("question-feed-subject-art") + .boundingBox(); + const expandedBox = await applet.boundingBox(); + expect(subjectBox).not.toBeNull(); + expect(expandedBox).not.toBeNull(); + expect( + boxesIntersect( + subjectBox as { x: number; y: number; width: number; height: number }, + expandedBox as { + x: number; + y: number; + width: number; + height: number; + } + ) + ).toBe(false); + + // Collapsing again hides the credit without unmounting the applet. + await applet.getByTestId("artist-support-toggle").click(); + await expect(applet.getByTestId("artist-support-credit")).toHaveCount(0); + }); + // Issue #503 (WTC phase C2) / #524 - supersedes this describe block's former "selecting a // grouped candidate submits the identical payload to the identical endpoint as an ungrouped // one" test (see .github/coverage-acks.txt for the rename ack). That title asserted phase