feat(gallery): brand the gallery shell like the website - #1383
Conversation
|
Design rationale: why the token NAMES stayed and only the values moved The obvious way to give the gallery the website's look is to give it the website's tokens, Every demo under One trap inside that mapping, worth knowing about if anyone touches the palette later. shadcn's On the logo. I expected to need an inline SVG with the gradient tokens, since That variant is why On |
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff fresh. The shape holds up: keeping the shadcn token names and moving only their values is what lets a full rebrand touch four files instead of forty, and the payload boundary is genuinely respected.
Seven things came back. Five are real and fixed in 73f22c9.
Two of them share a theme, and it is the one worth remembering: porting the website's tokens wholesale without porting their consumers, then writing a comment asserting the dead thing is load-bearing. Five shadow tokens (--shadow-sm, --shadow-cast, --shadow-ambient, --shadow-spread, --shadow) and three @theme mappings (--color-accent-tint, --color-hover-surface, --color-glow-a) were defined and read by nothing, and each set carried a comment claiming otherwise. The @theme one named a shadow-DOM demo as its consumer; that demo reads --color-border and --color-ring, neither of them new. A comment that justifies dead code is worse than the dead code, because the next reader trusts it. Both sets are gone, and --shadow-spread being dead had in turn made half the per-theme override block dead. Those two findings are anchored path-level rather than inline, since the fix removed the lines they sat on.
The storage-key finding is the most useful one here, and it is on its line below.
One finding I only half accept, also noted on its line. And one is a real CI gap that predates this branch: the Conventions job runs the three apps in a bash -e loop, so pre-existing submitter-needs-bound-form failures in examples/blog abort it before gallery is ever checked. Nothing on this branch causes it and the three red jobs are identical to those on the base commit, so I have flagged it rather than folded it in. I did run webjs check and webjs doctor against the gallery locally, both clean.
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, scoped to the previous round's fix commit and its blast radius. Four findings, all real, all fixed in 480e5d1.
The important one is a bug that predates this branch and that the previous round's own audit walked straight past. --destructive and --destructive-foreground are mapped in public/input.css and used at 20 call sites for validation errors, and nothing ever defined them. An undefined var() is invalid at computed-value time, so every error message in the auth, forms, file-storage, rate-limit and server-action demos painted in the ordinary foreground colour, and the destructive button variant had no fill at all. Measured in the browser: text-destructive computed byte-identical to body text before, and resolves to the red now. I folded it in rather than leaving it, because it is the same dead-token class I had just audited that exact block for, and because "only the token VALUES moved" was not true while three names had no values.
--color-success went the other way and is deleted. Nothing uses success in any form, so defining it would have added exactly the kind of dead token the last round removed three of.
The finding I want to flag hardest is the one against my own documentation. The @theme versus @theme inline rule I added to the styling reference, and leaned on in this PR's description, was wrong. I have rewritten it against a measurement instead of an assumption, and the table in it is what tailwindcss@4.3.0 actually does. Details on its line below.
Nothing else in the fix commit's radius moved: webjs_theme is now the single key across the gallery, the blog, the generated app and the website, and the removed tokens have no remaining reader.
vivek7405
left a comment
There was a problem hiding this comment.
Third pass, scoped to the previous fix commit. Two findings, both real, both fixed in 2af8351.
This round also re-derived the Tailwind emission rule independently, from an isolated project against the repo's own tailwindcss@4.3.0, and every cell of the corrected table holds, including the sub-claims that a css template literal inside a .ts file counts as scanned source and that both block forms drop an unreferenced token. Worth recording that the reviewer's first attempt at that measurement wrote its output inside a scanned directory and produced false positives, which is a good reminder that this particular question is easy to measure wrongly. That is exactly how the original bad rule got written.
The sharper of the two findings is that the last commit deleted a token for being dead and added an equally dead one in the same diff. --destructive-foreground is read by nothing here, because the destructive button variant is transparent at rest and colours its text. It is gone, so only --destructive is defined, which 15 call sites do read.
The second is a fair charge of arbitrariness that I accept in full. I removed --color-success for being dead while six other mapped-but-unused tokens sat beside it untouched, which is not a principle, it is a coin flip, and it put an unrelated pre-existing cleanup inside a branding PR. Reverted. input.css is now a one-comment change.
Those six remaining dead mappings are real but pre-existing and out of scope here. Flagging rather than folding in.
vivek7405
left a comment
There was a problem hiding this comment.
Fourth pass, scoped to the previous fix commit. Two findings, no code change from either.
Rejected, out of scope. The --color-success mapping's comment does claim consumers that do not exist, and that is a real defect, but it is not this PR's. The previous commit's restore cancelled the delete before it, so the block is byte-identical to main: git diff origin/main -- gallery/public/input.css shows only the one corrected --primary-tint comment and nothing about success. The finding reads it as added because it looked at that commit alone rather than the branch's net diff. Fixing it here would be the same arbitrary pre-existing cleanup I accepted the criticism for last round, so it goes to the owner instead, with the six mapped-but-unused tokens beside it.
Accepted as a correction to the record, with no branch change. I wrote "15 call sites" in the previous commit message. The real figure is 12, across 9 files, and the overcount came from the grep matching prose: the two mentions inside the comment that commit itself adds, plus the invitation to a filled variant in components/ui/button.ts:56. Counting a comment I had just written as evidence for the claim that comment makes is a circular measurement, and it is the third time on this PR that a number or a mechanism went in unmeasured. The shipped comment in layout.ts asserts no count and is accurate, so nothing on the branch is wrong; the commit message is, and it is pushed, so this note is the correction rather than a force-push that would break the commit links in the earlier rounds.
For the record, verified afresh this round: exactly 12 occurrences, in app/features/{file-storage,forms}/page.ts, app/features/auth/{login,signup}/page.ts, components/ui/button.ts, lib/utils/cn.ts, modules/components/components/task-loader.ts, modules/rate-limit/components/rate-probe.ts and modules/server-actions/components/clock-reader.ts.
This round also re-derived the Tailwind emission table a third time, independently, and it holds. Its own first attempt hit the same scan-root contamination the previous round hit, which is now three measurements agreeing and two false starts from the identical cause.
vivek7405
left a comment
There was a problem hiding this comment.
Final pass over the whole diff. Three findings, all real, all fixed in 8a88c1c. This round also independently re-derived the Tailwind emission table, checked the copied font and lockup blobs against git hash-object (byte-identical), and computed WCAG contrast for the new palette (no regression).
The sidebar. app/features/layout.ts pins at top: 1.5rem, measured against an in-flow navbar, so under the fixed header it pinned 33px behind the bar. The offset now resolves against --header-h. That file is payload, so the compensation lives in the root layout.
Worth recording what I found while diagnosing, because it changed the conclusion twice. The sidebar also rides out of view on a short page, and that is NOT caused by the fixed header: the payload caps it at calc(100dvh - 7.5rem), so it is nearly as tall as its own grid row and has about 10px of travel before the row runs out. I took the header out of normal flow to test the hypothesis and the back link landed within one pixel of where it had been, so I put it back. The finding was right about the pin offset and wrong about the mechanism, and I would have shipped the wrong fix if I had trusted the diagnosis instead of measuring it.
The theme-key fix had no test, which is a fair hit: it is the one live bug this PR fixes and AGENTS.md wants the counterfactual. A runner cannot observe it, since by the time any assertion runs the toggle has upgraded and the two keys agree again, so test/repo-health/gallery-theme-key.test.mjs asserts the invariant statically instead, plus a second test pinning the gallery to the repo-wide key. Reverting the layout to theme reds it.
The @theme rule reached only the agent skill, not website/app/docs/styling, which is the surface an app author actually reads and which still presented input.css as one plain @theme block. Added there with the measured table.
Verified after all three: scaffold output unchanged apart from the one doc file, repo-health 122/122, webjs check clean on gallery and website, doctor unchanged, and the website boots 200 on /, /docs/styling and /ui with no broken preloads.
gallery.webjs.dev and webjs.dev read as two different products. The favicon was reconciled recently, but everything below the tab strip still diverged: a cool-grey palette, Bricolage Grotesque off the Google Fonts CDN, no logo, no footer, and an in-flow header. Port the website's visual system to the gallery-only surfaces: the warm oklch palette, the self-hosted Inter Tight / Inter / JetBrains Mono stack, the ambient glow background, the fixed blurred header, the brand lockup, and a reduced footer. Both themes, and the logo swaps correctly with JavaScript off. The scaffold keeps shipping the neutral look. gallery/ is the single source of the feature gallery that webjs create copies into every generated app, and the neutral palette there is deliberate: an app-building agent reads the demos for idioms, and a vividly branded example outweighs a prose instruction to pick its own design. Only app/layout.ts, app/page.ts, components/theme-toggle.ts and public/ are gallery-only, so the change is confined to those and every demo, module and ui primitive is untouched. The shadcn token NAMES are kept and only their VALUES move, which is what makes that confinement possible: every payload demo is written against bg-card, text-muted-foreground and border-border, so they render branded with zero payload edits. shadcn's --accent stays neutral on purpose, since it is a hover surface rather than the brand accent despite sharing the website's name. Also fixes a live bug the port surfaced. The toggle wrote localStorage webjs_theme while the layout's bootstrap read theme, so a reader who picked a theme got a first paint in the other one.
bg-clip-text paints the gradient only inside the element box. leading-none makes that box exactly the font size, so a descender sits below it and, with text-transparent, renders as nothing. The g and the y in "gallery" were cut off at the baseline. It only became visible when the display face moved to Inter Tight, whose descenders are deeper relative to the em box than Bricolage Grotesque's, but the geometry was always wrong. Give the line box room to contain them.
gallery/ is both the single source of the feature gallery webjs create copies into every generated app and a live deployed app, and the two roles want opposite things from its design. Both hold only because a narrow set of files is gallery-only, and nothing enforced that split: the copy filter could stop filtering, a brand helper could be added under gallery/lib/, or the two copiers' directory lists could drift, and every existing suite would stay green while WebJs branding shipped into every generated app. Three assertions, one per regression. Note the first cannot cover lib/utils/cn.ts: the gallery's copy and the generated one are both read verbatim from packages/ui/packages/registry/lib/utils.ts, so they are identical whether the filter runs or not and the check would be tautological. The other three shell files carry generator-only content and discriminate.
The first assertion was tautological and its counterfactual caught it. It compared the generated app's shell files against the gallery's copies, and stayed green with copyGallery()'s filter removed entirely. The reason is that the filter is not what protects the boundary. The generator writes its own app/layout.ts, app/page.ts and components/theme-toggle.ts AFTER copyGallery() runs, so they overwrite whatever the copy left, and the fourth shell file is byte-identical to the gallery's because both are read verbatim from the ui registry. Neutering the filter changes nothing observable about the generated output. So assert the mechanism that is load-bearing: those three writes must follow the copy. Moving copyGallery() below them now fails, which is the regression that would actually ship the branded shell into every generated app. The predicate itself is covered too, both directions.
Three surfaces described the four-file shell exclusion but not the other half of the boundary, which this change leans on hardest: all of gallery/public/ is gallery-only too, because both copiers iterate only app, modules, test, components and lib. That is where the gallery's brand assets can live without reaching a generated app. Also record what actually holds the boundary. The generator writes its own shell files AFTER copyGallery() runs, so they overwrite the copy; the shell-file filter is defence-in-depth on top of that, not the mechanism. Documenting the filter as the mechanism is what let a tautological guard look correct. The styling reference gains the @theme versus @theme inline rule, which was written down nowhere and is silent when wrong. Only a plain @theme emits a real custom property, so a token read from inside a shadow component's static styles (which Tailwind cannot see) must be mapped that way or the rule falls back to currentColor.
Review fixes, all in the gallery-only shell. Five shadow tokens were defined and read by nothing. --shadow-sm, --shadow-cast, --shadow-ambient, --shadow-spread and --shadow were ported wholesale from the website without porting a consumer, and two of them also shadowed names Tailwind already owns. --shadow-spread being dead in turn made half the per-theme override block dead, while the comment above it claimed that block was needed. Removed, and the comment now describes what is left. Three @theme mappings went the same way. --color-accent-tint, --color-hover-surface and --color-glow-a had no consumer in any form, and the comment justifying them named a shadow-DOM demo that reads --color-border and --color-ring instead, neither of which is new. The layout's own consumers read the raw custom properties and bypass the mappings entirely. The storage key is fixed at the other end. The toggle and the bootstrap did disagree, but webjs_theme is the key every other consumer uses: the generated app create.js writes, examples/blog, and website's THEME_STORAGE_KEY constant. Moving the toggle onto `theme` fixed the flash while making the gallery the only app in the repo off the shared key, so the layout moves onto webjs_theme instead and one key holds repo-wide. Also: the toggle's comment claimed the .dark class is what the dark: variants read, which the new @custom-variant in input.css falsifies for this app; the preload comments said "two critical faces" while three are preloaded (mono earns its slot here, since the demo count and every card badge are monospace above the fold); and the first section heading reads Feature Demos rather than Feature Cards, which named the widget instead of the content.
Delta-review fixes. --destructive and --destructive-foreground were mapped in public/input.css and used at 20 call sites for validation errors, but nothing ever defined them. An undefined var() is invalid at computed-value time, so every error message in the auth, forms, file-storage, rate-limit and server-action demos painted in the ordinary foreground colour, and the destructive button variant had no fill. Measured in the browser before and after: text-destructive was byte-identical to body text, and now resolves to the red. Pre-existing, but it is the same dead-token class the previous commit audited, in the same block, and it undercut this PR's claim that only token VALUES moved. --color-success went the other way and is removed. Nothing in the app uses success in any form, so defining it would have added exactly the kind of dead token the last round removed three of. The styling reference's new rule was wrong and is rewritten against a measurement rather than an assumption. A component's static styles IS inside the scanned source, so a raw var(--color-x) written there forces emission even under @theme inline; and the emission rule is about HOW a token is referenced, not about which block declares it. Both forms drop a token nothing references. The cell that actually bites is inline plus utility-only usage. The gallery's plain @theme is therefore a fine choice but not, as previously written, a load-bearing one, and the shadow demo was never at risk. Also corrects two comments this PR had made stale: the toggle's .dark mirror runs only on click while the bootstrap writes data-theme alone, so the class is absent on every page load and is not the kit-compatibility path the comment claimed; and --primary-tint now derives from --ring, not --primary.
The previous commit deleted --color-success for being dead and then added --destructive-foreground, which is equally dead. Nothing under gallery/ reads it in any form: the destructive button variant is deliberately transparent at rest and colours its TEXT, so it uses text-muted-foreground with a hover:text-destructive, and Tailwind drops --color-destructive-foreground from the compiled output entirely. Removed, so only --destructive is defined, which 15 call sites do read. The --color-success deletion is reverted too. It is pre-existing, inert, and six other mapped-but-unused tokens sit beside it, so removing that one and leaving the rest was arbitrary rather than principled, and it put an unrelated pre-existing cleanup inside a branding PR. The net change to input.css is now one corrected comment.
Final-review fixes. app/features/layout.ts pins its sidebar at top: 1.5rem, a number its own comment measures against an in-flow navbar. Under this app's fixed header that pins 33px UNDER the bar, so wherever a page is long enough to hold the pin the back link sits behind the blur. The offset is now expressed against --header-h, scoped to the lg breakpoint where the payload shows the aside at all. The sidebar itself is payload and could not be touched. Measured while diagnosing, and worth recording because it looks like this change caused it: the sidebar also rides out of view on a short page, because the payload caps it at calc(100dvh - 7.5rem), leaving it nearly as tall as its own grid row with about 10px of travel. That is identical with the header in flow or out of it, within a pixel, so it is inherent to the payload layout rather than something this branch introduced. Taking the header out of flow was tried and changed nothing, so the header stays fixed. The theme-key fix now has a test. It is a first-paint timing property, which a runner cannot observe reliably (by the time an assertion runs the toggle has upgraded and the two agree), so the invariant is asserted statically on the source, and a second test pins the gallery to the repo-wide key rather than letting it drift back to a private one. Reverting the layout to the old key reds the first test. The @theme versus @theme inline rule reaches the docs site too, which is the surface an app author actually reads. It only landed on the agent skill before, while the docs page still presented input.css as a single plain @theme block and never mentioned that the scaffold emits @theme inline or that the two differ in whether the token reaches :root.
8a88c1c to
a46e41c
Compare
Closes #1382
Summary
Give
gallery.webjs.devthe website's visual identity whilewebjs createkeeps emitting the neutral look it emits today. The gallery's palette, fonts, background, header, logo and footer now matchwebjs.devin both themes, and the generated-app manifest is byte-identical to before the change.The constraint that shapes the whole PR is that
gallery/does double duty. It is the single canonical source of the feature gallery thatwebjs createcopies into every scaffolded app, AND a live deployed app. The neutral palette in the demo material is deliberate: an app-building agent reads the gallery for idioms then runsnpm run gallery:clear, and a vividly branded example outweighs a prose instruction telling it to choose its own design.Only five surfaces are gallery-only, and the change is confined to them:
Everything else under
gallery/app/,gallery/components/,gallery/lib/,gallery/modules/andgallery/test/is scaffold payload and is untouched.What changed
bg-card,text-muted-foreground,border-borderandbg-primary, so they all render branded with zero payload edits.gallery/public/fonts/, replacing three Google Fonts tags. The@font-faceblock inpublic/input.cssalready pointed at/public/fonts/and was 404ing, so this completes wiring that was half-written.gallery/public/brand/, swapped by adark:variant, which is exactly the mechanismwebsite/lib/design/brand.tsuses.https://webjs.dev/...URLs.localStoragekeywebjs_themewhile the layout's bootstrap readtheme, so a reader who picked a theme got a first paint in the other one.Deliberately not done
--accentstays neutral. It is a hover SURFACE and shares a name with the website's brand accent while meaning the opposite thing. Mapping it across would turn every hover surface in every payload demo orange.gallery/lib/orgallery/components/. The reflex is to mirrorwebsite/lib/design/brand.tsandwebsite/lib/ui/site-footer.ts. Both directories are payload, so that would ship the WebJs lockup and footer into every generated app. Both are inline in the layout instead.gallery/public/input.csskeeps ONE plain@themeblock and gains no@theme inline. The website's split exists for its.ui-previewsubtree, which the gallery has no equivalent of, soinlinewould buy nothing here. An earlier version of this description claimed the plain block was also load-bearing, becauseinlinewould strip the tokens a shadow-DOM demo reads off:root. That was wrong, and measuring it is what showed why: Tailwind scans source files as raw text, so thevar(--color-border)inside that component'sstatic stylesforces emission under either form. The demo was never at risk. The corrected rule, with the measured table, is in.agents/skills/webjs/references/styling.md.Test plan
.agents/skills/webjs/references/styling.md, which is the agent skill the scaffold ships and which this PR adds the@themeversus@theme inlinerule to. That is deliberate: the rule is framework guidance an app author with a shadow component needs, and it carries no branding. Excluding it, the two manifests are byte-identical, so no visual or structural byte of the scaffold moved.test/repo-health/gallery-payload-boundary.test.mjsadded, 3/3node --test 'test/repo-health/*.test.mjs'120/120node --test 'test/scaffolds/*.test.js'65/65( cd gallery && npm test )node 2/2, browser 4/4( cd gallery && npm run typecheck )clean( cd gallery && npx webjs check )all checks pass( cd gallery && npx webjs doctor )11 passed, 2 warnings, 0 failed. Both warnings are byte-for-byte the same onmain(ELISION_CARRIERS on three payload feature pages, STATIC_ASSET_FRESHNESS on the gitignoredtailwind.css), so neither is introduced here.gallery:clearon a generated app removes 43 paths, leaves no brand marker, and behaves identically tomainfonts.googleapis.comLayers that do not apply: e2e (no routing, navigation or streaming), Bun parity (nothing under
packages/*/srcis touched, so the parity hook does not trip), smoke (coversexamples/blog), and package unit tests (no package source changes).The first counterfactual failed, and that was the point
The guard originally compared a generated app's shell files against the gallery's copies. That assertion stayed green with
copyGallery()'s filter removed entirely, so it was tautological.The filter is not what protects the boundary. The generator writes its own
app/layout.ts,app/page.tsandcomponents/theme-toggle.tsAFTERcopyGallery()runs, so they overwrite whatever the copy left, andlib/utils/cn.tsis byte-identical to the gallery's because both are read verbatim frompackages/ui/packages/registry/lib/utils.ts. Neutering the filter changes nothing observable about the generated output.So the assertion now covers the mechanism that is load-bearing: those three writes must follow the copy. Moving
copyGallery()below them fails the test. The filter remains as defence-in-depth for the publishedtemplates/gallery/bundle, and the docs no longer describe it as the mechanism.