-
Notifications
You must be signed in to change notification settings - Fork 10
💄 Redesign /products index + third-party brand card tokens #4984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hveraus
wants to merge
19
commits into
main
Choose a base branch
from
Refine-products-index-page
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
ada9831
💄 Redesign /products index + third-party brand card tokens
hveraus 8c0f6bc
⚡ perf: shrink the YakShaver gradient asset and match its declared size
hveraus b6c255c
💄 Refine /products card treatment and fix the no-URL card regression
hveraus 255b83d
📝 Update the TinaCMS product card description
hveraus bb333ca
Merge remote-tracking branch 'origin/main' into Refine-products-index…
hveraus 8f72845
💄 Add capability tags to /products cards and soften their chrome
hveraus b1549ea
Merge remote-tracking branch 'origin/main' into Refine-products-index…
Copilot f6aff09
fix: restore tina-lock.json with tags field lost in merge conflict re…
Copilot 01f7f01
fix: restore events presenter role in tina lock
Copilot 4837779
🐛 fix: regenerate tina-lock.json so its schema matches the source
hveraus 73b0897
📝 Reorder and update /products card tags
hveraus 1c1c1f3
💄 Cap /products cards to two visible tags
hveraus cb0f27f
🐛 fix: pin Turbopack workspace root for local dev
hveraus 4609e59
📝 Tighten product descriptions and fix Rewards URL
hveraus 2e57dc8
♻️ Address PR review comments on /products
hveraus ec18b52
Merge remote-tracking branch 'origin/main' into Refine-products-index…
Copilot fb33bc3
🐛 fix: regenerate tina-lock.json after Copilot's main merge
hveraus e096cf5
Merge branch 'main' into Refine-products-index-page
hveraus b725e3b
🔥 Remove "More products coming" placeholder from /products
hveraus File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| import { ProductCard } from "@/components/products/productCard"; | ||
| import { render, screen } from "@testing-library/react"; | ||
| // This repo leaves setupFilesAfterEnv commented out in jest.config.ts, so the | ||
| // jest-dom matchers are not registered globally. Import them here. | ||
| import "@testing-library/jest-dom"; | ||
|
|
||
| // tinacms/dist/react is published as ESM and Jest does not transform | ||
| // node_modules, so importing it here fails to parse. These tests pass no | ||
| // tinaNode, which means tinaField is never actually called - the mock exists | ||
| // only to keep the module graph resolvable. | ||
| jest.mock("tinacms/dist/react", () => ({ | ||
| tinaField: () => undefined, | ||
| })); | ||
|
|
||
| // Regression guard for the card shell. | ||
| // | ||
| // CustomLink returns a bare fragment when href is falsy, which drops the element | ||
| // carrying the card's chrome classes. Because `url` is optional on productsList | ||
| // in tina/collections/products.tsx, an editor can save a product without one, | ||
| // and the card's children would then be emitted as siblings - becoming loose | ||
| // grid items in the products grid instead of one card. ProductCardShell renders | ||
| // a plain div in that case; these tests pin both halves of that behaviour. | ||
|
|
||
| const withUrl = { | ||
| name: "SugarLearning", | ||
| url: "https://sugarlearning.com/", | ||
| description: "Induction and onboarding.", | ||
| }; | ||
|
|
||
| const withoutUrl = { | ||
| name: "SugarLearning", | ||
| description: "Induction and onboarding.", | ||
| }; | ||
|
|
||
| // The chrome that must land on the card's single root element either way. | ||
| const SHELL_CLASSES = ["flex", "h-full", "flex-col", "rounded-card"]; | ||
|
|
||
| describe("ProductCard shell", () => { | ||
| test("renders an anchor carrying the card chrome when the product has a url", () => { | ||
| const { container } = render(<ProductCard product={withUrl} />); | ||
|
|
||
| const root = container.firstElementChild as HTMLElement; | ||
| expect(root.tagName).toBe("A"); | ||
| expect(root).toHaveAttribute("href", withUrl.url); | ||
| SHELL_CLASSES.forEach((c) => expect(root).toHaveClass(c)); | ||
| }); | ||
|
|
||
| test("still renders ONE root element carrying the card chrome when url is missing", () => { | ||
| const { container } = render(<ProductCard product={withoutUrl} />); | ||
|
|
||
| // The regression this guards against was the children being emitted as | ||
| // siblings, so assert on the child count before anything else. | ||
| expect(container.children).toHaveLength(1); | ||
|
|
||
| const root = container.firstElementChild as HTMLElement; | ||
| expect(root.tagName).toBe("DIV"); | ||
| expect(root).not.toHaveAttribute("href"); | ||
| SHELL_CLASSES.forEach((c) => expect(root).toHaveClass(c)); | ||
| }); | ||
|
|
||
| test("keeps the name and description inside that root element", () => { | ||
| const { container } = render(<ProductCard product={withoutUrl} />); | ||
| const root = container.firstElementChild as HTMLElement; | ||
|
|
||
| expect(root).toContainElement( | ||
| screen.getByRole("heading", { name: withoutUrl.name }) | ||
| ); | ||
| expect(root).toContainElement(screen.getByText(withoutUrl.description)); | ||
| }); | ||
|
|
||
| test("shows the destination hostname only when there is a url", () => { | ||
| const { unmount } = render(<ProductCard product={withUrl} />); | ||
| expect(screen.getByText("sugarlearning.com")).toBeInTheDocument(); | ||
| unmount(); | ||
|
|
||
| // destinationLabel returns "" for a missing url and the footer only renders | ||
| // the label when truthy, so no hostname should appear. | ||
| render(<ProductCard product={withoutUrl} />); | ||
| expect(screen.queryByText("sugarlearning.com")).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| // Tags come from a Tina `list: true` string field, so the component has to cope | ||
| // with the array being absent, holding blanks an editor left behind, or holding | ||
| // more entries than a card can show without changing height. | ||
| describe("ProductCard tags", () => { | ||
| const tagNames = (list: HTMLElement) => | ||
| Array.from(list.querySelectorAll("li")).map((li) => li.textContent); | ||
|
|
||
| test("renders each tag as a pill", () => { | ||
| render( | ||
| <ProductCard | ||
| product={{ ...withUrl, tags: ["Onboarding", "Induction"] }} | ||
| /> | ||
| ); | ||
|
|
||
| const list = screen.getByRole("list"); | ||
| expect(tagNames(list)).toEqual(["Onboarding", "Induction"]); | ||
|
|
||
| // The chip's own type scale has to land on the element, not just its colour. | ||
| // An earlier revision used the custom `text-xxs` key, which tailwind-merge | ||
| // silently dropped as a conflict with the text-colour class beside it, so | ||
| // the chips rendered at the inherited size; this pins the size class as a | ||
| // cheap guard against that class of regression coming back. | ||
| expect(list.querySelector("li")).toHaveClass("text-xs"); | ||
| }); | ||
|
|
||
| test("renders no list at all when there are no tags", () => { | ||
| const { unmount } = render(<ProductCard product={withUrl} />); | ||
| expect(screen.queryByRole("list")).toBeNull(); | ||
| unmount(); | ||
|
|
||
| // Blank entries are dropped, so a list of only blanks is the same as none - | ||
| // an empty <ul> would otherwise add its gap to the card for nothing. | ||
| render(<ProductCard product={{ ...withUrl, tags: ["", " "] }} />); | ||
| expect(screen.queryByRole("list")).toBeNull(); | ||
| }); | ||
|
|
||
| test("shows at most two tags so cards keep a predictable height", () => { | ||
| render( | ||
| <ProductCard | ||
| product={{ ...withUrl, tags: ["One", "Two", "Three", "Four"] }} | ||
| /> | ||
| ); | ||
|
|
||
| expect(tagNames(screen.getByRole("list"))).toEqual(["One", "Two"]); | ||
| expect(screen.queryByText("Three")).toBeNull(); | ||
| expect(screen.queryByText("Four")).toBeNull(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,37 +1,153 @@ | ||
| import { PageCard } from "@/components/blocks/pageCards"; | ||
| import { HomeThemeShell } from "@/components/layout/homeTheme"; | ||
| import { MoreProductsPanel } from "@/components/products/moreProductsPanel"; | ||
| import { ProductCard } from "@/components/products/productCard"; | ||
| import { TinaProductCard } from "@/components/products/tinaProductCard"; | ||
| import { YakShaverProductCard } from "@/components/products/yakShaverProductCard"; | ||
| import { Container } from "@/components/util/container"; | ||
| import { Breadcrumbs } from "app/components/breadcrumb"; | ||
| import { FC } from "react"; | ||
| import { tinaField } from "tinacms/dist/react"; | ||
|
|
||
| type BrandCardProps = { | ||
| product: { | ||
| name?: string; | ||
| url?: string; | ||
| description?: string; | ||
| tags?: string[]; | ||
| }; | ||
| tinaNode?: Record<string, unknown>; | ||
| }; | ||
|
|
||
| // The two products whose owners' media kits require their own card surface, | ||
| // keyed by lowercase name so the treatment follows the product if an editor | ||
| // reorders the CMS list. cellCount and the render below both read from this | ||
| // one map, so it cannot drift the way two separately-maintained lists could. | ||
| const BRAND_CARD_COMPONENTS: Record<string, FC<BrandCardProps>> = { | ||
| tinacms: TinaProductCard, | ||
| yakshaver: YakShaverProductCard, | ||
| }; | ||
|
|
||
| const brandCardFor = (name?: string) => | ||
| BRAND_CARD_COMPONENTS[(name ?? "").trim().toLowerCase()]; | ||
|
|
||
| export default function ProductsIndexContent({ props }) { | ||
| const node = props.productsIndex; | ||
|
|
||
| // Brand cards are pinned to the front of the render order, independent of | ||
| // where the CMS list actually puts them. `.sort` is stable, so this only | ||
| // reorders brand cards ahead of standard ones and otherwise preserves the | ||
| // CMS order on both sides of that split. | ||
| // | ||
| // This is load-bearing for cellCount below, not cosmetic: CSS Grid's default | ||
| // sparse auto-placement bumps a col-span-2 card that doesn't fit the | ||
| // remaining columns of its row to a fresh row, leaving the skipped cells | ||
| // empty rather than backfilling them from later cards. cellCount has no way | ||
| // to see that empty cell, so if a brand card ever landed mid-row the panel | ||
| // would compute a span that doesn't fit the real remaining slots and wrap to | ||
| // a row of its own - the exact ragged edge the panel exists to prevent. | ||
| // Pinning brand cards first guarantees they always open a row at a multiple | ||
| // of their own span (2), so this can't happen. | ||
| const products = [...(node?.productsList ?? [])].sort( | ||
| (a, b) => Number(!!brandCardFor(b?.name)) - Number(!!brandCardFor(a?.name)) | ||
| ); | ||
|
|
||
| // Each brand card occupies two grid cells at the tiers where it spans, so the | ||
| // trailing gap the panel fills is measured in cells, not products. | ||
| const cellCount = | ||
| products.length + products.filter((p) => brandCardFor(p?.name)).length; | ||
|
|
||
| return ( | ||
| <> | ||
| <Container className="mb-10 flex-1 pt-2"> | ||
| <Breadcrumbs path={"/products"} title={"Products"} /> | ||
| {props.productsIndex.title && ( | ||
| // min-h-screen, not min-h-full: PageLayout's <main> carries an | ||
| // unconditional bg-white, so any shortfall would show as a white band | ||
| // beneath the themed content in dark mode. | ||
| // | ||
| // bg-sunken-glow is the same page background /consulting uses, so the two | ||
| // index pages match: the sunken surface (#fafafa light, black dark) plus a | ||
| // faint red glow bleeding in from the top-right. The flat colour is baked | ||
| // into that token as a second gradient layer precisely so this works as a | ||
| // single class — pairing a `bg-*` colour with a `bg-*` image in one cn() | ||
| // makes tailwind-merge drop the colour. | ||
| <HomeThemeShell className="min-h-screen bg-sunken-glow"> | ||
| {/* Geometry deliberately identical to /consulting's page wrapper | ||
| (app/consulting/index.tsx): max-w-8xl, px-6 / max-md:px-3, and the same | ||
| vertical padding, so the two index pages line their breadcrumb, title | ||
| and content edges up exactly. size/width="custom" switch off | ||
| Container's own defaults (max-w-9xl and py-12), which is what differed | ||
| before — this page was 3rem wider with different gutters. */} | ||
| <Container | ||
| size="custom" | ||
| width="custom" | ||
| padding="px-6 max-md:px-3" | ||
| className="max-w-8xl pb-16 pt-4 max-md:pb-12 max-md:pt-3" | ||
| > | ||
| <div className="min-h-12"> | ||
| <Breadcrumbs path={"/products"} title={"Products"} /> | ||
| </div> | ||
|
|
||
| {node?.title && ( | ||
| <h1 | ||
| props-tina-field={tinaField(props.productsIndex, "title")} | ||
| className="mb-0 py-0 text-3xl" | ||
| // data-tina-field, not props-tina-field: Tina's visual editing | ||
| // looks for the data- attribute, so the previous spelling never | ||
| // registered a click target. | ||
| data-tina-field={tinaField(node, "title")} | ||
| // Type scale and box model both kept in step with /consulting's h1 | ||
| // (the `headingClass` constant in app/consulting/index.tsx), so the | ||
| // two index pages render the title identically. | ||
| // | ||
| // `m-0 p-0`, not `mb-0 py-0`: styles.css gives every h1 `my-4 pb-5 | ||
| // pt-15`, and zeroing only the bottom/vertical parts left an 18px | ||
| // top margin that /consulting does not have — enough to push this | ||
| // title out of alignment with theirs. `leading-tight` is omitted | ||
| // because styles.css already applies it to every h1-h5. | ||
| // | ||
| // `max-md:mt-2` reproduces the 9px that /consulting's title sits | ||
| // lower by below md, where its h1 lives inside a sticky chip-row | ||
| // wrapper carrying `max-md:pt-2`. There is no chip row here, so the | ||
| // offset has to be stated directly to keep the two titles aligned. | ||
| className="m-0 p-0 text-xl font-semibold text-foreground max-md:mt-2 max-md:text-lg xl:text-2xl" | ||
| > | ||
| {props.productsIndex.title} | ||
| {node.title} | ||
| </h1> | ||
| )} | ||
| {props.productsIndex.subTitle && ( | ||
| <h2 | ||
| props-tina-field={tinaField(props.productsIndex, "subTitle")} | ||
| className="mb-4 text-base" | ||
| > | ||
| {props.productsIndex.subTitle} | ||
| </h2> | ||
| )} | ||
| <div className="flex flex-col md:flex-row"> | ||
| <div className="grid w-full grid-cols-1 gap-2 lg:grid-cols-2"> | ||
| {props.productsIndex.productsList?.map((product, index) => ( | ||
| <PageCard page={product} key={index} /> | ||
| ))} | ||
| </div> | ||
| <div | ||
| // grid-cols-N in Tailwind is already repeat(N, minmax(0, 1fr)), which | ||
| // is what keeps a wide logo from blowing out a track. | ||
| // | ||
| // Breakpoints: the design asks for 4-up at 1240px and 2-up at 760px. | ||
| // Those aren't breakpoints this theme defines, so this uses the | ||
| // nearest ones it does — xl (1280) and md (768) — rather than adding | ||
| // two one-off screens to the config. | ||
| // | ||
| // mt-8 replaces the gap the subtitle's own bottom margin used to | ||
| // provide, so the grid doesn't butt up against the title. | ||
| className="mt-8 grid grid-cols-1 gap-4 md:grid-cols-2 md:gap-6 xl:grid-cols-4" | ||
| > | ||
| {products.map((product, index) => { | ||
| const key = `${product?.name ?? "product"}-${index}`; | ||
| const BrandCard = brandCardFor(product?.name); | ||
|
|
||
| // Brand cards span two columns from md up. The base tier stays at | ||
| // span 1: a col-span-2 in the single-column grid would add an | ||
| // implicit second column and cause horizontal scroll. | ||
| if (BrandCard) { | ||
| return ( | ||
| <div key={key} className="md:col-span-2"> | ||
| <BrandCard product={product} tinaNode={product} /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <ProductCard key={key} product={product} tinaNode={product} /> | ||
| ); | ||
| })} | ||
|
|
||
| <MoreProductsPanel | ||
| cellsAtMidTier={cellCount} | ||
| cellsAtWidestTier={cellCount} | ||
| /> | ||
| </div> | ||
| </Container> | ||
| </> | ||
| </HomeThemeShell> | ||
| ); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { cn } from "@/lib/utils"; | ||
| import { FC } from "react"; | ||
|
|
||
| // Tailwind's JIT only generates classes it can see as literal strings in the | ||
| // source, so the computed spans are looked up in these maps rather than built | ||
| // by interpolation — `xl:col-span-${n}` would compile to nothing. | ||
| // | ||
| // Below md the grid is a single column and every card already fills its row, so | ||
| // the default span is correct and no base-tier map is needed. | ||
| const MID_SPAN_CLASS: Record<number, string> = { | ||
| 1: "md:col-span-1", | ||
| 2: "md:col-span-2", | ||
| }; | ||
| const WIDE_SPAN_CLASS: Record<number, string> = { | ||
| 1: "xl:col-span-1", | ||
| 2: "xl:col-span-2", | ||
| 3: "xl:col-span-3", | ||
| 4: "xl:col-span-4", | ||
| }; | ||
|
|
||
| type MoreProductsPanelProps = { | ||
| // Grid cells the products ahead of this panel occupy, counting the two brand | ||
| // cards as 2 each at the tiers where they span two columns. | ||
| cellsAtMidTier: number; | ||
| cellsAtWidestTier: number; | ||
| }; | ||
|
|
||
| // Fills the gap left at the end of the last row so the grid doesn't end on a | ||
| // ragged edge. The span is derived from how many cells the products occupy at | ||
| // each tier rather than hardcoded, so it stays correct as products are added to | ||
| // or removed from the CMS. | ||
| export const MoreProductsPanel: FC<MoreProductsPanelProps> = ({ | ||
| cellsAtMidTier, | ||
| cellsAtWidestTier, | ||
| }) => { | ||
| // A remainder of 0 means the last row is already full, so the panel takes a | ||
| // whole row of its own rather than collapsing to zero width. | ||
| const spanFor = (cells: number, columns: number) => { | ||
| const remainder = cells % columns; | ||
| return remainder === 0 ? columns : columns - remainder; | ||
| }; | ||
|
|
||
| return ( | ||
| <div | ||
| className={cn( | ||
| // Dashed, and deliberately without hover or lift: this is a state, not | ||
| // a destination. | ||
| // No gap utility: this holds a single line, so a gap doesn't apply. | ||
| "flex min-h-24 flex-col items-center justify-center rounded-card border-0.75 border-dashed border-stroke-weak p-6 text-center dark:border-hairline", | ||
| MID_SPAN_CLASS[spanFor(cellsAtMidTier, 2)], | ||
| WIDE_SPAN_CLASS[spanFor(cellsAtWidestTier, 4)] | ||
| )} | ||
| > | ||
| <p className="m-0 p-0 text-sm font-medium text-foreground"> | ||
| More products coming | ||
| </p> | ||
| </div> | ||
| ); | ||
| }; |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth making the ordering explicit. This is exact only because TinaCMS and YakShaver happen to lead the CMS list.
A
col-span-2card that cannot fit the remaining columns is bumped to the next row and leaves the rest of the current row empty (sparse auto-placement, the default).cellCountnever sees that hole. I simulated placement across orderings: with the current order the panel span is right at both tiers, but move any standard card in front of a brand card and real cells go 13 to 14, so at 4-up the panel computes span 3 into 2 free slots, cannot fit, and wraps to a row of its own - the ragged trailing row the panel exists to prevent.Sorting brand cards to the front before the map is one line, makes this count provably exact (2 divides both 2 and 4, so brand-first never leaves a hole), and changes nothing visually today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the correctness bug the reviewer flagged in app/(about)/products/products-index.tsx: cellCount was only accurate because TinaCMS and YakShaver happen to lead the CMS list today. Under CSS Grid's default sparse auto-placement, a col-span-2 brand card that doesn't fit the remaining columns of its row gets bumped to a fresh row, leaving the skipped cell empty — cellCount's arithmetic can't see that gap, so a reorder in the CMS would make the trailing MoreProductsPanel compute the wrong span and wrap into its own ragged row.
Fix: sort brand cards to the front of the render order before computing cellCount and mapping the grid, using a stable sort so CMS order is otherwise preserved on both sides of the split. Verified this makes the count provably correct (2 divides both the 2-col and 4-col breakpoints, so brand-first can never leave a gap) and changes nothing visually today — confirmed the live-rendered grid order is exactly TinaCMS, YakShaver, SugarLearning, EagleEye, TimePro, SophieBot, SophieHub, SSW Dory, CodeAuditor, SSW Rewards, SmashingBarrier, unchanged from the CMS order.
Bonus catch along the way: while verifying this, I hit a page-breaking error (Cannot read properties of null (reading 'data')) that turned out to be unrelated to this edit — the manual tinacms build I ran earlier for the subTitle fix had left the running dev server's Turbopack/Tina cache stale. Traced it by querying the local GraphQL server directly and calling the generated query function standalone (both succeeded, proving the schema/content were fine), then restarted the dev server to clear the stale cache. It's now healthy and serving the page correctly.