Skip to content

feat: add theme builder and contrast tooling to color palette generator - #5157

Draft
alexlvrs wants to merge 4 commits into
mainfrom
feat/color-palette-generator-updates
Draft

feat: add theme builder and contrast tooling to color palette generator#5157
alexlvrs wants to merge 4 commits into
mainfrom
feat/color-palette-generator-updates

Conversation

@alexlvrs

@alexlvrs alexlvrs commented Jul 9, 2026

Copy link
Copy Markdown

What this does

Updates the Color Palette Generator app (apps/eds-color-palette-generator):

  • Theme builder + tooling — adds the theme builder as the main page, plus contrast checker and palette editor routes (a71f92b1).
  • Lint fixes — resolves 6 react-hooks ESLint errors + 1 warning that surfaced when the theme-builder code moved to /; behaviour-preserving (00aeec26).
  • Restructure — promotes the theme builder to the root route, moves the legacy generator to /old, reorganises components into docs/ old/ shared/ themebuilder/, removes unused contrast helpers, updates e2e specs (f80938bd).

Verified locally: lint 0 errors / 0 warnings, types clean, test:run 126/126, and the app was driven end-to-end (theme builder tabs, contrast table, donut chart, component preview, /example, /palette) with no console or hydration errors.

⚠️ Open questions for the reviewer (need a developer's call)

Two items are intentionally not in this PR — left in the local working tree pending your decision (so they won't show in the diff):

  1. pnpm-lock.yaml — the local checkout has a ~500-line lockfile churn (native binary variants) with no package.json change. Looks like an install artifact, not intentional. Recommendation: discard (git checkout pnpm-lock.yaml) unless deliberate.
  2. plan/*.prompt.md (fix-lint-failures, restructure-palette-generator) — untracked prompt docs. Commit them as documentation, or leave them out? Your call.

Notes

  • Target main. This app is not a release-please-tracked package, so merging won't trigger a version bump.

alexlvrs added 3 commits July 8, 2026 14:47
Resolve pre-existing ESLint errors surfaced by the theme-builder promotion:
- ContrastTable/ComponentPreviewPanel: call hooks before early returns
- DataColorChart: precompute donut segments instead of mutating during render
- example/page.tsx: hydrate custom palettes via useSyncExternalStore (SSR-safe)
- palette/page.tsx: annotate one-shot client-only hydration; drop unused import
Promote the theme builder to the root route and move the legacy generator
to /old. Reorganize components into docs/, old/, shared/, and themebuilder/
folders; adjust contrast/about/layout routes; drop unused contrast helpers;
update e2e specs for the new routes.

Includes the render-time clamp fix in the root page (safeContrastIndex),
which depends on the promoted theme-builder code and so lands here.
@alexlvrs
alexlvrs requested a review from eddiman July 9, 2026 10:06
@alexlvrs

alexlvrs commented Jul 9, 2026

Copy link
Copy Markdown
Author

Poke @eddiman has I wasn't sure about this thingy :)

@pomfrida

pomfrida commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

@claude review

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude finished @pomfrida's task in 2m 30s —— View job


Code Review — Color Palette Generator theme builder

  • Read the PR diff and understand the changes
  • Review theme builder + contrast tooling
  • Review shared utils (color, palette, urlState)
  • Check accessibility, correctness, and conventions
  • Post review feedback

Reviewed against origin/main. This is an internal app (not a release-tracked package), so the correctness/a11y bar is lower than for the component library — but a few things are worth your attention. Overall the restructure is clean and the anchor-preservation change in color.ts looks sensible.


🔴 Dependency downgrades in package.json (likely the real cause of the lockfile churn)

apps/eds-color-palette-generator/package.json downgrades a batch of dependencies relative to main and removes one:

Package main this PR
next / eslint-config-next / @next/eslint-plugin-next 16.2.9 16.2.4
react / react-dom ^19.2.6 ^19.2.4
@types/react ^19.2.15 ^19.2.14
lucide-react ^1.21.0 ^1.12.0
@playwright/test ^1.61.1 ^1.59.1
tailwindcss / @tailwindcss/postcss ^4.3.1 ^4.2.4
vite / vite-plugin-dts / vitest ^8.1.0 / ^5.0.3 / ^4.1.9 ^8.0.10 / ^4.5.4 / ^4.1.5
tsx ^4.22.4 ^4.21.0
@microsoft/api-extractor present removed

Your PR description says the lockfile churn has "no package.json change" — but there is a committed package.json change here, and it's a broad downgrade. This looks like the branch was created before main picked up the dependency bumps (e.g. 7b3005e chore(deps): bump the all-dependencies group) and never re-synced. This is almost certainly what's generating the ~500-line lockfile delta, and committing a downgraded package.json without the matching lockfile will break pnpm install --frozen-lockfile in CI.

Recommend restoring these to match main unless the downgrade is deliberate (and if it is, say why in the PR). Fix this →


🟠 Correctness

urlState.ts — delimiters in names/values aren't escaped. serializePalette joins palettes with ,, anchors with @, and uses : / = as separators, but the palette name is written raw (only deserializePalette calls decodeURIComponent). A palette named Blue, Green or containing :/@ will round-trip incorrectly. encodeURIComponent the name (and anchor value) on serialize to match the decode side. (urlState.ts:31-39)

updateURL debounce timer is never cleared on unmount. The module-level debounceTimer (urlState.ts:128-142) fires 300 ms after the last change with no cleanup in the page.tsx effect, so a trailing replaceState can run after navigation away. Return a cleanup from the effect, or expose a cancel. (page.tsx:61-67)

Index-based React keys with editable/removable rows. PaletteInputPanel maps palettes with key={i} (PaletteInputPanel.tsx:122) while AnchorRow holds per-row local state (localValue, isValid) and the parent tracks expandedIndex by position. Removing/reordering a palette shifts indices, so local input state and the expanded row bind to the wrong palette. Prefer a stable id per palette/anchor. Fix this →

ContrastTable palette buttons keyed by p.name (ContrastTable.tsx:72) — two palettes with the same name collide. Also fine to use a stable id.


🟡 Accessibility

  • Icon-only buttons rely on title alone (remove X, expand chevron, Pipette) in PaletteInputPanel.tsx and SimpleColorPicker.tsx. title is not a reliable accessible name — add aria-label. The anchor <select> and value <input> do correctly use aria-label, so just mirror that on the buttons. Fix this →
  • Unlabelled text inputs: the palette-name input (PaletteInputPanel.tsx:126, placeholder only) and the hex input in SimpleColorPicker.tsx:17 have no <label>/aria-label.
  • Good: the contrast badges encode the level as text (AA/FAIL/DECO), not colour alone. 👍

🟢 Minor / nits

  • ContrastTestPanel.tsx:22const stablePalettes = useMemo(() => palettes, [palettes]) is a no-op (returns the same reference with itself as the only dep); it stabilises nothing. Drop it and use palettes directly.
  • next.config.ts /themebuilder → / redirect is a reasonable temporary shim; consider a follow-up to update the Share button to emit / directly so the redirect can be removed.
  • On the two open questions in the description: discard the lockfile churn only after resolving the package.json downgrade above (they're linked). The plan/*.prompt.md docs are a judgement call — I'd leave build-agent prompt scratch files out of the app unless the team wants them tracked.

Nice work on the restructure and the anchor-exact-color fix. The dependency downgrade is the one thing I'd resolve before merge; the rest are app-quality polish you can take or leave.
· feat/color-palette-generator-updates

Reorganisation (behaviour-preserving):
- Rename colliding config files (palette.ts -> palette-config.ts,
  dataviz.ts -> dataviz-defaults.ts) and update all importers.
- Break up the oversized route files into feature components:
  about 866->55, example 1012->117, contrast 795->151, palette 485->199.
- Extract shared logic to utils (contrastPageData, customPalettesStore,
  generatorImport) and add per-feature barrel exports.

Also includes in-progress dataviz / colour-vision-deficiency tooling
(new /dataviz route, CVD simulation, semantic colours).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants