feat(skills): design picker UI (Phase 1 templates index + Phase 2 fine-tune)#974
feat(skills): design picker UI (Phase 1 templates index + Phase 2 fine-tune)#974vanceingalls wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
jrusso1020
left a comment
There was a problem hiding this comment.
Read the structural surface of design-picker.html (7192 lines, single file). Per the rubric this is one cohesive surface — focused review on phase boundaries, the token-injection scheme, and cross-PR consistency rather than a CSS/JS line-by-line walk. Posting as COMMENT (not stamping) per merge policy.
Phase boundary verification
Lines 1554-1556 declare the phase tabs (tab-mood → Phase 1 "Template", tab-tune → Phase 2 "Fine-tune"). Lines 1566-1582 declare the corresponding phase containers (#phase-mood is display:active initially, #phase-tune hidden via the .phase not-.active rule at line 104-110). The Phase 1 / Phase 2 split described in the PR body matches the implementation boundaries — no Phase 2 surface leaking into Phase 1's render path, no unfinished hooks I could spot. ✓
The "Phase 1 → Phase 2" handoff documented in the PR body (picks.palette = tpActivePal + scroll-to-top on template change) is the right shape for state passthrough.
Cross-PR token-substitution consistency
Verified the __X_JSON__ / __BASE_HREF__ placeholders in design-picker.html against build-design-picker.py's substitutions in #973:
Picker-side placeholders consumed by the build script (matched 1:1):
__BASE_HREF__✓__PALETTES_JSON__✓__TEMPLATES_JSON__✓__ARCHITECTURES_JSON__✓__MOODBOARDS_JSON__✓__PROMPT_JSON__✓__PROMPT_TEXT_JSON__✓__TYPEPAIRS_JSON__✓
One mismatch flagged on #973's review: __PROMPT_DESC__ is substituted by the build script but does NOT appear in design-picker.html. Silent no-op currently. Either drop from the schema or add the placeholder where the prompt description should land. Tracked on #973; mentioning here for cross-stack visibility.
Other __*__ placeholders in the picker HTML (__PRIMARY__, __ACCENT__, __BG_TYPE__, __EASING_DESC__, __SHADER_VERTEX__, etc.) are not build-script substitutions — they're literal sentinels that travel into the design.html iframe contents and get substituted client-side at runtime per the PR body's "client-side token substitution + blob URL" claim. Cross-stack consistency holds as long as the matching design.html files in #972 don't carry orphan sentinels at render time. PR #972's test plan calls out a manual visual check for this — please make sure that happens.
Hot-spots review
tpApplyAllPalettestoken injection — the documented behavior (injecting both--tp-*and--primary/--secondary/--tertiary/--accenton iframedocumentElement, while stripping--tp-accentfrom scoped template CSS) is the right shape to avoid circularvar()references. Subtle correctness path; worth verifying once with DevTools (compute the resolved--accentvalue on a.demo-cardafter a palette change — should resolve via the iframedocumentElementstyle attribute, not from a circular cascade).- Shader uniform rebake via
_lastBgModeKeyreset — described in PR body. The rebake-on-palette-change is the right shape; the risk is "shader uses palette" toggle ↔ "shader uses custom three colors" toggle clobbering the rebake key. Worth a manual check. tpScaleIframeselector matches both.idx-spec-previewand.cprev— documented as legacy-compat. Worth a follow-up to grep the codebase for.cprevusage and drop the alias once nothing references it._tpNameSplitname-splitting heuristic — handlesBlockFrame→Block / Frame,People's Platform (Block & Bold)etc. Edge cases worth pinning by test if any are ever surfaced as bugs.
Non-blocking
- 7192 lines in one file is the cost of the framework-less approach (intentional per the PR body). Worth documenting why somewhere in the file header — future maintainers will reach for "split into modules" as the obvious cleanup, and the answer is probably "framework-less is intentional, keep it one file." A 3-line top-of-file comment captures that.
- External CDN dependency on
cdn.jsdelivr.netfor GSAP + Three.js (lines 8-17). For an offline-capable picker this is fine since the picker is local-only via127.0.0.1; would only matter if running on a network-isolated host. Probably not worth changing for an "open the picker, point at fonts.googleapis.com anyway" surface. - No new tests for the picker JS logic. The Phase 1 ↔ Phase 2 handoff (and the shader-preset-preserves-config behavior across palette changes) are the most-likely regression vectors. Hard to unit-test 7k of vanilla JS in a single file; a Playwright smoke test (
open the picker, pick a palette, verify--accentresolves on the iframe doc) would lock in the highest-value invariants. Not a blocker.
Scope question
The PR title says "Phase 1 templates index + Phase 2 fine-tune" — both phases land here. That's correct given the picker is one HTML file, but worth confirming whether Vance considered splitting Phase 1 / Phase 2 into separate PRs for incremental review (probably not feasible given the shared JS state — agreed with the current shape).
No blockers from my side on this layer. The picker is a substantial chunk of work and the cross-PR consistency holds up.
— Rames Jusso
2aca5e9 to
b909b29
Compare
e298640 to
1f2a797
Compare
b909b29 to
44c8912
Compare
1f2a797 to
91bc7ea
Compare
…e-tune) design-picker.html is the picker shell — a single HTML file with two phases. Phase 1 (templates index): editorial-catalog layout (Source Serif 4 italic + Hanken Grotesk on warm-tinted OKLCH neutrals). Sticky compact palette bar at the top. 3-up specimen grid where each card renders the template's design.html cover via client-side token substitution + blob URL. Roman-numeral folio markers, hairline rules. Phase 2 (fine-tune): sidebar with palette / typography / corners / density / depth / motion / background shader sections. Right pane shows the chosen template's design.html in an iframe with --primary / --secondary / --tertiary / --accent / --tp-* tokens injected on every control change. Shader background uses three.js (postprocessing EffectComposer with optional HalftonePass). Surface example card reacts only to corners/density/depth picks — does not leak to other surfaces. Per-swatch text color computed from luminance so palette previews stay legible. Motion change re-runs hero entrance animation and scrolls the iframe preview to the top. Phase 1 palette pick carried forward into Phase 2.
91bc7ea to
cedf30b
Compare

Summary
The picker UI itself — a single ~6k-line HTML file with two phases. Built by
build-design-picker.py(PR #973), opened byhyperframes pick(PR #975).Scope
skills/hyperframes/templates/design-picker.htmlPhases
Phase 1 — Templates index
Editorial-catalog layout. Source Serif 4 italic + Hanken Grotesk on warm-tinted OKLCH neutrals. Roman-numeral folio markers, hairline rules, generous space.
i. Name · 4-cell swatch strip, horizontally scrollable, ~60px tall (much shorter than a chip card).design.htmlcover (from PR feat(skills): hand-crafted design.html for each template #972) via client-side token substitution + blob URL. No card borders, no shadows, hairline rule under the preview, italic tagline below.transform: scale(containerWidth/1920).i. — xxxiv.) and01 / 34counters in the gutter.Phase 2 — Fine-tune
Sidebar with the picker controls + a right pane showing the chosen template's
design.htmlin an iframe. Picker controls inject--primary/--secondary/--tertiary/--accent/--tp-*tokens directly on the iframe'sdocumentElementon every change.Sections:
.demo-cardonly (no leakage to other surfaces).window._sgRenderPer-swatch text color
Each palette swatch's text color is computed from the bg luminance so previews stay legible regardless of palette. Inserted at picker-render time by
build-design-picker.py(PR #973).Phase 1 → Phase 2
Palette pick from Phase 1 is carried forward into Phase 2 (
picks.palette = tpActivePal). Sidebar scrolls to top + iframe preview scrolls to top on every template change.Hot spots for review
tpApplyAllPalettesinjects both--tp-*and the--primary/--secondary/--tertiary/--accentdesign.html tokens on the iframe document.--tp-accentis intentionally stripped from the scoped template CSS so cascade inheritance flows fromhtmlstyle attribute →.ds-slide-framewithout circularvar()references._lastBgModeKeyis reset after everyrenderDesignIframecall so palette changes force the shader uniforms to re-bake from the current palette viagetBgColors().tpScaleIframeselector. Matches both.idx-spec-preview(new editorial layout) and legacy.cprevso existing callers still work._tpNameSplit— splits template names on" (", then space, then CamelCase. HandlesBlockFrame→Block / Frame,People's Platform (Block & Bold)→People's Platform / (Block & Bold).What's not in scope
design.htmlfiles the picker renders — those are PR feat(skills): hand-crafted design.html for each template #972Test plan
hyperframes pick(from PR feat(cli): hyperframes pick — open the design picker, gated behind feature flag #975) opens the picker.demo-cardin the Surface sectionStack
Depends on #971, #972, #973. Followed by #975 (CLI).
🤖 Generated with Claude Code