💄 Redesign /products index + third-party brand card tokens - #4984
💄 Redesign /products index + third-party brand card tokens#4984hveraus wants to merge 23 commits into
Conversation
Bring /products in line with the /consulting index redesigned in #4939: same page background, container geometry, title scale and card interaction model. - Opt /products into home theming via THEMED_ROUTES, so it picks up the sunken-glow background and dark-mode surfaces. - Split the page into per-product card components under components/products/, with the shared card chrome (geometry, hover, focus) in shared.ts. - Give TinaCMS and YakShaver their own branded cards, spanning two columns from md up. Matched on product name so the treatment follows the product if an editor reorders the CMS list. - Add MoreProductsPanel to fill the trailing gap on the last grid row, sized from the occupied cell count rather than hardcoded. - Namespace TinaCMS brand colours as global --brand-tina-* tokens in styles.css, documenting the WCAG AA contrast measurement behind each value, and expose them through tailwind.config.js. - Fix the title's Tina visual-editing attribute: props-tina-field was never a real attribute, so the h1 never registered as a click target. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage report
Test suite run success35 tests passing in 4 suites. Report generated by 🧪jest coverage report action from a0776a1 |
There was a problem hiding this comment.
Pull request overview
Updates the /products index to match the redesigned /consulting index by opting into home theming, introducing a shared card interaction model, and adding branded card treatments for third-party products.
Changes:
- Opts
/productsinto home theming and updates page wrapper/geometry to match/consulting. - Replaces the old
PageCardgrid with dedicated product card components (standard + branded TinaCMS/YakShaver) and shared card chrome. - Adds global TinaCMS brand CSS tokens (documented for contrast) and exposes them to Tailwind utilities.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tailwind.config.js | Exposes TinaCMS brand CSS variables via Tailwind color tokens. |
| styles.css | Introduces namespaced third-party brand tokens for TinaCMS (with contrast notes). |
| public/images/company-logos/TinaCms-Logo-Full-Default.svg | Adds TinaCMS full lockup SVG for branded product card. |
| components/products/yakShaverProductCard.tsx | Adds YakShaver branded card component (always-dark scoped surface + artwork). |
| components/products/tinaProductCard.tsx | Adds TinaCMS branded card component using new brand tokens. |
| components/products/shared.ts | Introduces shared card shell classes and destination label helper. |
| components/products/productCard.tsx | Adds standard product card component using shared chrome and tokens. |
| components/products/moreProductsPanel.tsx | Adds grid “More products coming” filler panel with computed spans. |
| components/layout/homeTheme.tsx | Adds /products to THEMED_ROUTES so it receives home theming. |
| app/(about)/products/products-index.tsx | Rebuilds /products index layout and renders new card components + filler panel. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // overlay is unnecessary, which also means the focus outline can sit on the | ||
| // card itself again instead of on a child that overflow-hidden would clip. | ||
| <CustomLink | ||
| href={product.url ?? ""} |
| // (/products/rewards) through next/link and adds target/rel to the | ||
| // genuinely external ones. | ||
| <CustomLink | ||
| href={product.url ?? ""} |
| }) => { | ||
| return ( | ||
| <CustomLink | ||
| href={product.url ?? ""} |
hveraus
added
the
no-issue
Re-exports the decorative bloom at 500x320 instead of 1670x1069, taking the file from 1.1 MB to 164 KB. Aspect ratio is unchanged (1.5625 vs 1.5622), so the artwork renders identically. Also corrects the hardcoded width/height on the <Image>. next/image derives its srcset from `width`, so leaving 1670 there would have requested 1670w/3840w candidates of a 500px source and given back most of the saving. Addresses the asset-weight finding from the multi-agent review of #4984. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-exports the decorative bloom at 500x320 instead of 1670x1069, taking the file from 1.1 MB to 164 KB. Aspect ratio is unchanged (1.5625 vs 1.5622), so the artwork renders identically. Also corrects the hardcoded width/height on the <Image>. next/image derives its srcset from `width`, so leaving 1670 there would have requested 1670w/3840w candidates of a 500px source and given back most of the saving. Addresses the asset-weight finding from the multi-agent review of #4984. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
83efb6f to
8c0f6bc
Compare
Follow-up to the review of #4984. Fix - A product with no `url` lost its entire card. All three cards passed `product.url ?? ""` and CustomLink returns a bare fragment when href is falsy, discarding the wrapper along with its className, so the logo plate, copy and footer became loose grid items. `url` is optional in tina/collections/products.tsx, and the design this replaced guarded it via `{page.url && ...}` in pageCards.tsx. New ProductCardShell renders CustomLink when there is a href and the same shell as a plain div when there is not. Guarded here rather than in CustomLink on purpose: that component has ~99 call sites, several inside <p> elements, where emitting a <div> would be invalid HTML and a hydration mismatch. - Covered by __tests__/components/products/productCard.tsx, which fails on the previous implementation. jest.config.ts gains a `@/lib/*` moduleNameMapper entry so components importing cn() are testable at all. Brand cards (TinaCMS, YakShaver) - Dropped the arrow affordance and replaced the destination URL with a "Learn More" chip. The chip is a styled <span>, not a <button> or nested <a>, both of which are invalid inside the card's own <a>. - Chip grows to 105% on card hover; needed transition-all rather than transition-colors for the transform to ease. - Right padding raised to 40px (`pr-10`), kept after `p-6` in the cn() list because tailwind-merge drops a `pr-*` that precedes a `p-*`. - Tina llama watermark enlarged to h-3/4 and pushed to -bottom-4, so overflow-hidden crops the feet deliberately. Standard cards - Logo plate radius raised from rounded-utility (8px) to rounded-card (16px). - Arrow is now filled at rest and only turns and grows on hover, achieved by removing the overrides that forced an outline-to-fill effect and pinned group-hover:scale-100. ArrowCircle's own bg-foreground/text-background defaults give a dark circle in light mode and a white one in dark mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the copy on the /products TinaCMS card so it covers TinaDocs as well as the visual editor. Note the same sentence is still duplicated in content/megamenu/menu.json, which is deliberately left alone here - the mega menu is a separate surface, so the two now differ. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/deploy |
|
Deployed changes to https://app-sswwebsite-9eb3-pr-4984.azurewebsites.net ℹ️ Staging slots are no longer created automatically - comment |
Tags - New optional `tags` string list on productsList (tina/collections/products.tsx) so the copy stays editable in the CMS like every other product field, with three authored per product. At most three render; blanks and extras are dropped, so an editor can add a fourth without changing a card's height. - Rendered as sentence-case chips with a brand-red stroke and a 16% red wash. The stroke is what makes them read as chips: the neutral hairline they started with measured 1.25:1 against the light card and 1.38:1 against the dark one, where red measures 4.53:1 and 3.99:1. - The label stays neutral rather than going red with the stroke: text-brand is only 3.99:1 on the dark card at 12px, which fails AA. The TinaCMS card keeps white outline chips for the inverse reason - red measures 1.15:1 against its orange field, and a white wash would drop its label to 4.33:1. Chrome - Both brand cards' Learn More chips now come from one shared constant, so the near-black label is identical on each. The TinaCMS one previously used the darkened brand orange. - The footer arrow's disc no longer uses ArrowCircle's full-strength inversion, which measured 16.7:1 against the light card and 19.0:1 against the dark one - more contrast than the card title, for an affordance. Now 1.27:1 / 1.20:1, with the glyph gaining contrast as the disc fades. Overridden at the call site only; the v3 blocks still get the strong version. All contrast figures measured in a browser against the rendered surfaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@copilot resolve the merge conflicts in this pull request |
…-page # Conflicts: # tina/tina-lock.json Co-authored-by: hveraus <4745861+hveraus@users.noreply.github.com>
Merge conflicts resolved — the only conflict was in |
…solution Co-authored-by: hveraus <4745861+hveraus@users.noreply.github.com>
Co-authored-by: hveraus <4745861+hveraus@users.noreply.github.com>
The merge of main at b1549ea hand-resolved the conflict in this generated file and left its `eventsCalendar` collection at the pre-#4978 shape, so the lock disagreed with tina/collections/events-calendar.tsx in the same tree: committed lock 26 fields, including an obsolete `headerLayout` source schema 32 fields Missing from the lock were availability, bannerImage, ctaLabel, entryCost, lead, price and venue. `tinacms build --content=local` checks the local schema against the one TinaCloud indexed, so it exited 1 and took `pnpm run build` with it - surfacing only as the Docker `buildx failed` wrapper in CI, since the job logs are not readable without admin rights. Every `PR - build` run since that merge failed this way; the two follow-up commits that hand-patched this file did not fully restore it. Regenerated rather than hand-edited, which is the only reliable way to touch a 30MB generated file. Verified structurally against origin/main's lock: all 46 collections, the entire lookup table and all 20035 GraphQL definitions are now identical to main's, with the sole exception of productsIndex gaining the `tags` field this PR adds (schema plus ProductsIndexProductsList, its Filter and its Mutation). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/deploy |
|
Deployed changes to https://app-sswwebsite-9eb3-pr-4984.azurewebsites.net ℹ️ Staging slots are no longer created automatically - comment |
|
Deployed changes to https://app-sswwebsite-9eb3-pr-4984.azurewebsites.net ℹ️ Staging slots are no longer created automatically - comment |
🚀 Lighthouse Report
|
isaaclombardssw
left a comment
isaaclombardssw
left a comment
There was a problem hiding this comment.
🤖 Agent Review
Requesting changes on four things, all commented inline against the diff.
🚨 Must Fix / Address
- 💄 Comment volume - 477 of 888 lines in
components/products/are comment (54%), and two of them are already wrong. - SSW Rewards link - relative to absolute non-www, which costs the soft navigation and adds a 301 hop.
- tsconfig.json - no longer Prettier-clean where
mainwas, plus an undeclared repo-widemoduleResolution/jsxflip. - subTitle - still
required: trueand populated, but never rendered.
🤏 Small Feedback
- Make the brand-card ordering explicit rather than assumed.
BRAND_CARDSand the render branches state the same list twice.- "Fun Quizes" typo in the Rewards tags.
📞 Callouts
- The rest is in good shape - CI passes on this SHA, tests and lint are clean locally, and the
THEMED_ROUTESopt-in, thedata-tina-fieldfix and the falsy-href guard inProductCardShellare all nicely done. This is a request for changes on the list above only.
| @@ -0,0 +1,170 @@ | |||
| // Shared chrome for the three kinds of card on /products (standard, TinaCMS, | |||
There was a problem hiding this comment.
Comment volume. components/products/ is 477 of 888 lines comment (54%), and this file is 127 of 170 (74%). The baseline across components/ is about 4%.
The twMerge traps are worth keeping - I ran twMerge and confirmed outline-2 really does swallow the bare outline and silently kill the focus indicator. That is a genuine trap and the comment earns its place.
What I would cut is the revision narrative ("an earlier revision lifted the card 6px", "two intermediate revisions were still too strong") which git already stores, and the hand-measured contrast ratios, which nothing tests and which quietly become false the moment someone changes --card.
There was a problem hiding this comment.
done with Claude:
Removed revision narrative — sentences describing prior iterations (e.g. "an earlier revision lifted the card 6px," "two intermediate revisions were still too strong," the removed install-command chip / gradient blend-mode history). Git history already covers this.
Removed hardcoded contrast ratios — every manually-measured X.XX:1 value, since nothing tests them and they'd silently go stale if a --card/--brand token changes. Kept the accessibility intent instead (e.g. "clears WCAG 3:1," "would fail AA in dark mode").
Kept as-is: the outline-2/twMerge focus-indicator warning in shared.ts, plus all other why-comments explaining twMerge conflicts, library quirks, and non-obvious trade-offs.
Net diff: -142/+103 lines, comment-only (verified no non-comment lines touched), tests still pass. Also fixed a stale doc-comment in yakShaverProductCard.tsx that claimed the text column was width-capped when the actual code (and the per-element comment) shows it isn't.
| return ( | ||
| // ProductCardShell, not a raw <a>: for a product that has a url it wraps | ||
| // CustomLink, which routes the one on-site product (SSW Rewards, at | ||
| // https://www.ssw.com.au/products/rewards) through next/link and adds |
There was a problem hiding this comment.
This says https://www.ssw.com.au/products/rewards, but the content has no www - it is https://ssw.com.au/products/rewards. The missing www is exactly what causes the redirect hop flagged in index.json.
A second one at line 49: the chrome is not identical to ConsultingCard, which uses rounded-xl where this uses rounded-card. Two comments drifted from the code inside one PR is the argument for fewer of them.
There was a problem hiding this comment.
Addressed the comment and made the requested display change:
Fixed the stale/inaccurate comments they flagged (productCard.tsx):
The doc-comment referenced https://www.ssw.com.au/products/rewards — updated to https://ssw.com.au/products/rewards to match the actual content (no www).
The comment claiming ConsultingCard and this card have "identical card chrome" was wrong — ConsultingCard uses rounded-xl (12px), this uses rounded-card (16px). Reworded to state parity is on colour/motion only, not shape, so the comment can't drift from the code again.
Changed the displayed URL (shared.ts):
destinationLabel used to strip the path and show hostname only. It now keeps the path when the URL points below the domain root, so SSW Rewards' footer shows ssw.com.au/products/rewards instead of the ambiguous ssw.com.au (which read like a link to the homepage).
Side effect, not a regression: SophieBot's URL also has a non-root path (/sophiebot), so its footer label picked up the same fix and now shows sswsophie.com/sophiebot instead of just sswsophie.com. Every other product's URL is root-path, so their labels are unchanged.
| <PageCard page={product} key={index} /> | ||
| ))} | ||
| </div> | ||
| {/* The CMS `subTitle` is deliberately not rendered. It still holds |
There was a problem hiding this comment.
Please remove subTitle properly rather than dropping it at the render.
The reasoning here is that the field is required: true so it cannot be emptied - but this PR already edits tina/collections/products.tsx to add tags and regenerates tina-lock.json, so the collection is in play.
As it stands an editor sees a mandatory field, fills it in, and nothing happens on the page, with no signal anywhere in the CMS. Either drop required, or add a description saying it is not displayed - exactly what the new tags field does.
There was a problem hiding this comment.
Don't understand, but this is what Claude fixed:
Removed subTitle from the schema properly rather than leaving it dropped at render:
tina/collections/products.tsx — deleted the field definition entirely.
content/products/index/index.json — removed the orphaned subTitle key and its content.
app/(about)/products/products-index.tsx — removed the now-obsolete comment explaining the render-time drop.
tina/tina-lock.json — regenerated via tinacms build --content=local. Verified productsIndex now has ['seo', 'title', 'productsList'] (matches main minus subTitle), productsList still carries tags, and the total collection count is unchanged.
|
|
||
| // 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 = |
There was a problem hiding this comment.
Worth making the ordering explicit. This is exact only because TinaCMS and YakShaver happen to lead the CMS list.
A col-span-2 card 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). cellCount never 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.
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.
| // The two products whose owners' media kits require their own card surface. | ||
| // Matched on name rather than list position so the treatment follows the | ||
| // product if an editor reorders the CMS list. | ||
| const BRAND_CARDS = ["tinacms", "yakshaver"]; |
There was a problem hiding this comment.
Minor: this list drives cellCount, but rendering branches on hardcoded name === "tinacms" / "yakshaver" literals at lines 106 and 114. Hardcoding these two is completely fine - it is just stated in two places, so adding a third name here would inflate the panel span against a card that never actually spans.
One Record<string, FC> that both the count and the render read from would close it.
There was a problem hiding this comment.
Consolidated the brand-card lookup into a single source of truth, per the reviewer's suggestion, since the previous BRAND_CARDS array plus the two hardcoded name === "tinacms" / name === "yakshaver" render branches were three separately-maintained lists that cellCount and the JSX could silently drift apart from.
Change in app/(about)/products/products-index.tsx:
Replaced BRAND_CARDS/isBrandCard with BRAND_CARD_COMPONENTS: Record<string, FC> mapping "tinacms" → TinaProductCard and "yakshaver" → YakShaverProductCard, plus a brandCardFor(name) lookup.
The sort (brand-first ordering), cellCount, and the render's brand-card branch all now read from this one map — the two if (name === ...) branches collapsed into a single if (BrandCard) return <BrandCard .../>.
Kept it as a plain object rather than reaching for anything fancier, since it's still just two entries.
- Trim comment bloat in components/products/*: drop revision-narrative and hardcoded contrast-ratio comments, keep twMerge/why comments. - Fix stale comments referencing the old absolute Rewards URL and the false "identical chrome" claim vs ConsultingCard. - Revert SSW Rewards' url to the relative /products/rewards path used on main; destinationLabel now appends the path for on-site links so the footer still shows the full destination. - Remove the CMS subTitle field properly (schema, content, generated lock) instead of leaving it unrendered. - Fix products-index.tsx's brand-card ordering: pin brand cards first so cellCount's grid-cell arithmetic can't be thrown off by sparse auto-placement if the CMS list is ever reordered, and consolidate the brand-card name checks into one Record<string, FC> both cellCount and the render read from. - Revert tsconfig.json to main: its diff was Next's own dev-server auto-rewrite, unrelated to this PR's turbopack.root fix. - Fix "Quizes" typo in SSW Rewards' tags. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@copilot resolve the merge conflicts in this pull request |
…-page Co-authored-by: hveraus <4745861+hveraus@users.noreply.github.com>
Resolved and pushed in |
Copilot's conflict resolution when merging main into this branch left tina-lock.json out of sync with the merged collections schema (main added the Pills block among other changes). tinacms build --content=local audits the local schema against this lock, so it exited 1 and took npm run build with it — the same failure mode as the earlier 4837779 incident, just surfaced through Copilot's merge this time. Regenerated via tinacms build rather than hand-edited, since it's a 30MB generated file. Verified structurally: still 46 collections, the new Pills block schema from main is present, and productsIndex still matches this branch's changes (no subTitle, tags on productsList). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
/deploy |
|
Deployed changes to https://app-sswwebsite-9eb3-pr-4984.azurewebsites.net ℹ️ Staging slots are no longer created automatically - comment |
🚀 Lighthouse Report
|
isaaclombardssw
left a comment
isaaclombardssw
left a comment
There was a problem hiding this comment.
Please cut the comments right down - this is the one item from last round that measurably didn't move. components/products/ went from 40.1% comment lines to 39.6%, and shared.ts is still 119 comment lines to 34 of code. The contrast ratios moved to styles.css rather than going away. components/consulting/ next door is 7 comment lines across 117 lines of code.
About 10 lines across the whole folder is what I'd expect. Worth keeping: the border-0.75 note (shared.ts:9), the twMerge outline-2 trap (shared.ts:26), why the spans are looked up in a map instead of interpolated, and the contrast values in styles.css. The rest is restating the code, aesthetic rationale that belongs in the PR description, or revision narrative git already stores.
The risk isn't just noise. Nothing compiles or tests a comment, so volume is exposure - and two are wrong right now. shared.ts:144 says every product is authored with an absolute URL, which your own Rewards fix falsified this round, and it sits directly above the new URL(url, base) fallback telling the next person the base isn't needed. shared.ts:136 lists duplicates as a case visibleTags handles, but it doesn't, and key={tag} throws on a repeated tag.
Drops the trailing dashed panel and the grid-cell math that only existed to size it.
Comment volume: cut components/products/ from 282 comment lines (35.6%) down to 27 (6.8%) — 433 lines removed net. Kept only the load-bearing notes: the border-0.75 rationale and the outline/twMerge outline-2 trap in shared.ts, plus a few genuine invisible-breakage guards elsewhere (the CustomLink fragment-collapse reason in productCardShell.tsx, the dark-scope corollaries and pr-10-must-follow-p-6 twMerge trap on the two brand cards, and why the brand cards' titles are sr-only). Everything aesthetic/rationale/narrative was deleted. Left styles.css's contrast-ratio comments untouched, as the reviewer said to keep those. The two wrong comments, both fixed at the code level, not just reworded: shared.ts no longer claims every product has an absolute URL — SSW Rewards' /products/rewards is relative, which is exactly why destinationLabel takes a base URL; the comment now says that plainly. |
components/products/ had grown to ~36% comment lines, mostly aesthetic
rationale and revision narrative git already stores. Trims it down to
the load-bearing notes only.
Also fixes two comments that were flatly wrong: shared.ts claimed every
product has an absolute URL (SSW Rewards' /products/rewards is
relative) and claimed visibleTags handles duplicate tags when it
didn't - now it actually dedupes, which also removes the key={tag}
collision risk on a repeated tag.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
/deploy |
| // 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 |
There was a problem hiding this comment.
Dont think we need this file? tests/* anything?
Should it be .gitignored
There was a problem hiding this comment.
It's intentional, not stray — __tests__ is already an established, tracked pattern in this repo (e.g. __tests__/components/v2ComponentWrapper.test.tsx, __tests__/helpers/consultingSections.ts from earlier PRs), and .gitignore only excludes /test-results/ (Jest's output), not __tests__ (the source).
CI actually runs it: pr-tests.yml runs pnpm test-and-coverage (Jest) on every PR into main and posts a coverage report, so these need to be committed to be picked up.
This particular file is a regression guard, not incidental scaffolding — it pins the fix for a real bug where CustomLink drops the card's chrome classes when a Tina-authored product has no url, so the children get emitted as loose siblings in the grid instead of inside one card. Without the test, that could silently come back on a future refactor.
| <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 |
There was a problem hiding this comment.
Reduce all comments on this file (if possible remove)
There was a problem hiding this comment.
Trimmed in 2b7bc51 — cut the long design-rationale blocks down to short one-liners, kept only the ones guarding a real gotcha (dark-mode white band, the 18px h1 margin bug, horizontal scroll from col-span-2 at the base grid tier).
| @@ -0,0 +1,11 @@ | |||
| <svg width="1020" height="254" viewBox="0 0 1020 254" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
There was a problem hiding this comment.
pls dont write out the svg code
There was a problem hiding this comment.
This is the real TinaCMS asset, not hand-typed/generated code — the icon paths match TinaCMS's own npm package (@tinacms/app/favicon.svg) scaled up, and the 7 wordmark paths after it decode letter-by-letter into "t-i-n-a-c-m-s". SVG is just XML text, so any addition to this file shows as a markup diff in review regardless of source — there's no way to add an SVG without it appearing as "written out" code.
|
Deployed changes to https://app-sswwebsite-9eb3-pr-4984.azurewebsites.net ℹ️ Staging slots are no longer created automatically - comment |
🚀 Lighthouse Report
|
There was a problem hiding this comment.
pretty sure u can get the gradient in code from SSW.Products
There was a problem hiding this comment.
tried, but hard to get the size and angle I want
joshbermanssw
left a comment
joshbermanssw
left a comment
There was a problem hiding this comment.
Code quality
The visuals are fine. The problem is that this page is written in the CMS's vocabulary instead of the domain's, and it's hard to read as a result.
/products is a grid of products. The only noun it should need is product. Right now products-index.tsx says "node" twice before it says anything about a product, and every card takes a tinaNode prop typed Record<string, unknown>. That's CMS transport plumbing leaking into the view layer, and it's the root of most of what follows — four hand-written copies of one shape, 9 defensive ternaries, and an untyped entry point. Fix the types at the boundary and the rest collapses on its own.
Separately: the PR description no longer matches the branch. It still describes MoreProductsPanel and sizing it "from the occupied cell count rather than hardcoded" — both were deleted in b725e3b and 3e8135c and neither exists in the code. The template placeholders (- Affected routes:, Fixed #{{ ISSUE NUMBER }}) are also still unfilled.
| } | ||
|
|
||
| return ( | ||
| <ProductCard key={key} product={product} tinaNode={product} /> |
There was a problem hiding this comment.
A UI component should never take a tinaNode.
Both call sites — this line and :118 — pass product={product} tinaNode={product}. The same object, twice, under two names, the second typed Record<string, unknown>.
That prop exists only because the hand-written product type discards Tina's generated type and the _content_source metadata tinaField reads off the object. So a second escape-hatch prop was needed to hand that metadata back. It's any with extra steps.
product is the Tina node — that's precisely why both props receive the same value. Type product from the generated type and call tinaField(product, "name") directly, the way app/articles/index.tsx:56 already does.
What goes away: the prop, its type, 18 references, the 9 copies of tinaNode ? tinaField(tinaNode, "x") : undefined guarding a prop no call site omits, and three of the four duplicate shape types.
<ProductCard key={key} product={product} />There was a problem hiding this comment.
Done — product is now typed ProductsIndexProductsList (from @/tina/types) everywhere, and tinaNode is gone entirely: the prop, its type, all 18 references, and the 9 guarded tinaNode ? tinaField(tinaNode, "x") : undefined ternaries. tinaField(product, "...") is called directly now, same as app/articles/index.tsx:56.
Changed: products-index.tsx, productCard.tsx, tinaProductCard.tsx, yakShaverProductCard.tsx (types + call sites), plus the two / call sites that were passing the same object twice.
One behavioural note: tinaField returns "" (not undefined) when _content_source metadata is absent, so data-tina-field now renders as data-tina-field="" in that case instead of being omitted — consistent with how articles/index.tsx already behaves, not a regression.
| BRAND_CARD_COMPONENTS[(name ?? "").trim().toLowerCase()]; | ||
|
|
||
| export default function ProductsIndexContent({ props }) { | ||
| const node = props.productsIndex; |
There was a problem hiding this comment.
Same point at the entry: this page renders products, not nodes. node is CMS vocabulary and it's the first thing the file says.
Related, on the line above: export default function ProductsIndexContent({ props }) has an untyped parameter, so it's implicitly any under strict: false — and the prop is literally named props, so this reads props.props. Every access below it (props.productsIndex, node?.title, node?.productsList) is unchecked, and a field rename in the collection fails silently at runtime instead of at build.
There was a problem hiding this comment.
Fixed — ProductsIndexContent now takes { data }: { data: ProductsIndexQueryQuery } (the actual generated type from client.queries.productsIndexQuery, imported from @/tina/types), and node is renamed to productsIndex throughout. Updated the one call site (products-index-preview.tsx) to pass data={data} instead of props={data}.
Field accesses (productsIndex.title, productsIndex.productsList) are now type-checked against the real Tina schema, so a collection field rename would fail at build time, not silently at runtime.
Flagging one thing out of scope for this comment: products-index-preview.tsx's own { props } param has the identical untyped-props smell one layer up, and consulting/index.tsx still uses node for the same pattern — happy to do those as a follow-up if you'd like.
| // 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. | ||
| const BRAND_CARD_COMPONENTS: Record<string, FC<BrandCardProps>> = { |
There was a problem hiding this comment.
Record<string, FC<BrandCardProps>> throws away the keys — brandCardFor returns FC<BrandCardProps> | undefined and nothing knows the map only holds "tinacms" and "yakshaver".
satisfies keeps both the check and the inference:
const BRAND_CARD_COMPONENTS = {
tinacms: TinaProductCard,
yakshaver: YakShaverProductCard,
} satisfies Record<string, FC<BrandCardProps>>;There was a problem hiding this comment.
Applied exactly as suggested — BRAND_CARD_COMPONENTS now infers its literal key type ("tinacms" | "yakshaver") via satisfies Record<string, FC> instead of widening to Record<string, FC> and throwing the keys away. Reply draft:
Done — switched to satisfies exactly as you suggested, so the object keeps its literal key inference while still being checked against the Record<string, FC> shape.
|
|
||
| // YakShaver's card: their dark-mode lockup and brand gradient artwork on the | ||
| // same surface the other nine products use in dark mode. | ||
| export const YakShaverProductCard: FC<YakShaverProductCardProps> = ({ |
There was a problem hiding this comment.
This and tinaProductCard.tsx are the same component.
Normalised for names, the two files differ by roughly 15 lines across 112 and 103. Identical shell, identical sr-only h3 carrying the name, identical tags ul, identical "Learn More" span, identical ProductCardShell wrapper.
The actual difference is two image paths, one class string and one tag-chip token — that's a brand config object, not a second file. As it stands any fix to the shared structure has to be made twice and nothing catches it when it isn't.
There was a problem hiding this comment.
Done — tinaProductCard.tsx and yakShaverProductCard.tsx are gone, replaced by one components/products/brandProductCard.tsx with two config objects (TINA_CARD_CONFIG, YAKSHAVER_CARD_CONFIG). To be sure I didn't introduce any visual drift, I wrote a throwaway test rendering the old vs. new components with identical props and diffed the HTML — it caught a real regression on the first pass (I'd wrapped YakShaver's content in an extra flex div that wasn't there before, changing its gap spacing), which I fixed by making the content wrapper config-driven per brand. Final diff confirmed byte-identical markup for both cards, then I deleted the scratch test.
Merged into a single components/products/brandProductCard.tsx, config-driven (TINA_CARD_CONFIG / YAKSHAVER_CARD_CONFIG). products-index.tsx now renders instead of picking between two components.
Verified with a throwaway diff test (old component vs. new + config, same props, HTML compared) before deleting the two files — output is unchanged for both brands, including the no-url/no-description/no-tags edge cases.
| "border-hairline hover:border-brand", | ||
| "hover:bg-brand-tina-field-hover", | ||
| "focus-visible:outline-white", | ||
| // Must stay after p-6: twMerge lets a later p-* override an earlier |
There was a problem hiding this comment.
This comment is copy-pasted verbatim to yakShaverProductCard.tsx:49.
It documents a real ordering constraint — pr-10 must follow p-6 or twMerge drops it — but nothing enforces it. Reorder the classes in either file and the padding silently disappears, in both places, with the comment still sitting there saying it won't.
A constraint that matters enough to document twice is worth encoding once: compose the padding into a single token in shared.ts, or pin it with a test. The duplicated paragraph is the weakest of the three options.
There was a problem hiding this comment.
Fixed. That duplication is already gone — tinaProductCard.tsx/yakShaverProductCard.tsx were merged into a single brandProductCard.tsx in an earlier round, so this comment now exists exactly once, not twice.
Your deeper point still applied to that one copy though: nothing stopped someone from splitting p-6/pr-10 back into two reorderable strings later. Composed it into a single brandCardPadding token in shared.ts per your suggestion, so the ordering can't be silently broken by a future edit — verified the rendered class list still ends ...p-6 pr-10 (right override intact) with a quick render check.
| } from "./shared"; | ||
| import { ProductCardShell } from "./productCardShell"; | ||
|
|
||
| export type ProductCardProps = { |
There was a problem hiding this comment.
Fourth hand-written copy of the same shape — alongside BrandCardProps (products-index.tsx:10), TinaProductCardProps (tinaProductCard.tsx:11) and YakShaverProductCardProps (yakShaverProductCard.tsx:19).
All four restate a shape Tina already generates from the collection. Add a field to productsList and you edit four types by hand, and nothing fails if you miss one. Derive them from the generated type instead.
There was a problem hiding this comment.
Already resolved by the earlier fixes in this thread — down from four hand-written shapes to two, and both (ProductCardProps, BrandProductCardProps) are { product: ProductsIndexProductsList }, deriving from the generated type rather than restating it. A field added to productsList now flows through both automatically; nothing left to hand-edit or silently miss.
Josh flagged the comment volume in review — cut long design-rationale blocks down to short one-liners, keeping only the ones that guard a real gotcha (dark-mode white band, the 18px h1 margin bug, horizontal scroll from col-span-2 at the base tier).
…rand cards, tighten types - Remove the redundant tinaNode escape-hatch prop; type product from Tina's generated ProductsIndexProductsList and call tinaField(product, ...) directly, matching the pattern already used in app/articles/index.tsx. - Type ProductsIndexContent's entry point as ProductsIndexQueryQuery instead of an implicit-any props bag, and rename node -> productsIndex so a collection field rename fails at build time instead of silently at runtime. - Merge tinaProductCard.tsx and yakShaverProductCard.tsx into a single config-driven brandProductCard.tsx (verified byte-identical rendered output against both originals before deleting them). - satisfies Record<...> on the brand card config/component maps to keep literal key inference. - Compose the p-6/pr-10 twMerge ordering constraint into one brandCardPadding token in shared.ts so it can't be silently split apart later. - Trim excessive comment blocks in products-index.tsx and styles.css down to the handful that guard a real gotcha. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bring /products in line with the /consulting index redesigned in #4939: same page background, container geometry, title scale and card interaction model.
Affected routes:
Fixed #{{ ISSUE NUMBER }}