chore(elements): TileRow + sprite palette unification#78
Conversation
Processed barbarian through the full pipeline (palette merge, bg removal, 4x upscale). Also corrects sprite-pipeline skill: upscale is --scale 4, matching rogue/druid; removes incorrect note about 2x being sufficient. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ain.palette.json Reduces all sprites to ≤8 colors via greedy merge and standardizes the outline color to #030307 across all classes (barbarian, druid, fighter, wizard — rogue was canonical). Adds build-main-palette script and main.palette.json combining all 5 palettes (35 unique colors). Also fixes reduce-palette.mjs: allow --merge into external colors (not just existing palette entries) and add 1% fuzz to -opaque to catch single-pixel compression artifacts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces each sprite's near-black outline with the canonical #030307 (barbarian #140202, druid #092907, fighter #010217, wizard #050A12). Rogue was already canonical and is unchanged. Druid only: also collapses 3 genuine near-duplicate pairs (distance ≤15) that were artifacts of the generation, not intentional shadow work: #062205→#092907, #86A91D→#8BAC0F, #85AA3A→#8BAC0F. 12→9 colors. All other shadow/highlight colors preserved — no greedy target merges. Adds main.palette.json (43 unique colors across 5 sprites). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tier 1-2 (safe, d≤30): - skin highlight: fighter #ECBD9A → #EFB88B (barbarian canonical) - skin shadow: fighter #BA7A66 → #BA724D (barbarian canonical) - skin dark: wizard #704536 → #623C2A (fighter canonical) Tier 3 (judgment calls, d≤34, visually confirmed): - deep shadow: barbarian #511507 → #3B1F17 (rogue canonical) - druid staff: druid #433B15 → #3B1F17 - leather dark: rogue #683B1D → #832B10 (barbarian canonical) 43 → 37 unique colors in main.palette.json. Skin mid (#EA9364 vs #926446, d=104) intentionally NOT merged — genuinely different skin tones. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Consolidates 3 near-white/grey weapon highlight colors (#DADAD9 1px, #B5B5B4 5px) into #FFFFFE. Shadow and skin work fully preserved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces #CDFDB0 (green-tinted near-white, 12px) with #FFFFFE, aligning the staff orb highlight with barbarian's axe highlight. Druid now shares #FFFFFE with barbarian. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
#66 Replaces ad-hoc tile markup in ScorePicker, OriginBonusPicker, HitDie display, and ability badges with a single TileRow element. Interactive mode renders accessible buttons (aria-label, aria-pressed, aria-disabled); read-only mode renders a role="list" of spans. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new <TileRow /> UI primitive to unify repeated “compact tile row” markup across character-creation screens, and continues the sprite palette unification work by normalizing sprites/palettes and adding supporting scripts.
Changes:
- Add
<TileRow />(read-only + interactive modes) and refactor ScorePicker/character-creation steps to use it, removing duplicated CSS. - Add tests for TileRow and ScorePicker interaction/a11y behavior.
- Add/adjust sprite processing scripts and update sprite assets + per-sprite palettes + a generated
main.palette.json.
Reviewed changes
Copilot reviewed 28 out of 40 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/elements/tile-row/tile-row.tsx | New unified tile-row component used by multiple flows. |
| src/elements/tile-row/tile-row.test.tsx | Adds coverage for TileRow roles/aria and interactions. |
| src/elements/tile-row/tile-row.module.css | Shared styling for the “tile row” visual primitive. |
| src/elements/score-picker/score-picker.tsx | Refactors ScorePicker rendering to delegate to TileRow. |
| src/elements/score-picker/score-picker.test.tsx | Adds tests for ScorePicker behavior (now TileRow-backed). |
| src/elements/score-picker/score-picker.module.css | Removes old grid/tile styling; keeps placeholder styling. |
| src/elements/index.ts | Exports TileRow and TileItem from the elements barrel. |
| src/elements/elements-showcase.tsx | Demonstrates TileRow variants in the showcase. |
| src/components/character-creation/step-origin/step-origin.tsx | Replaces ability “badge” markup with TileRow. |
| src/components/character-creation/step-origin/step-origin.module.css | Removes now-redundant ability bonus badge styles. |
| src/components/character-creation/step-class/step-class.tsx | Replaces Hit Die options markup with TileRow. |
| src/components/character-creation/step-class/step-class.module.css | Removes now-redundant hit die option styles. |
| src/components/character-creation/step-abilities/step-abilities.module.css | Removes now-redundant origin bonus picker grid styles. |
| src/components/character-creation/step-abilities/origin-bonus-picker.tsx | Refactors origin bonus picker to use TileRow (+ right-click unpick). |
| scripts/sprite-merge-candidates.mjs | New script to find cross-sprite near-color merge candidates. |
| scripts/reduce-palette.mjs | Adjusts explicit merge behavior and ImageMagick replacement strategy. |
| scripts/build-main-palette.mjs | New script to build a combined main.palette.json from per-sprite palettes. |
| public/assets/sprites/barbarian.png | Updates processed barbarian sprite asset. |
| public/assets/sprites/barbarian_4x.png | Adds/updates 4x upscaled barbarian sprite asset. |
| public/assets/sprites/barbarian-palette.json | Adds/updates barbarian palette JSON. |
| public/assets/sprites/druid.png | Updates processed druid sprite asset. |
| public/assets/sprites/druid_4x.png | Adds/updates 4x upscaled druid sprite asset. |
| public/assets/sprites/druid-palette.json | Updates druid palette JSON after normalization. |
| public/assets/sprites/fighter.png | Updates processed fighter sprite asset. |
| public/assets/sprites/fighter_4x.png | Adds/updates 4x upscaled fighter sprite asset. |
| public/assets/sprites/fighter-palette.json | Updates fighter palette JSON after normalization. |
| public/assets/sprites/rogue.png | Updates processed rogue sprite asset. |
| public/assets/sprites/rogue_4x.png | Adds/updates 4x upscaled rogue sprite asset. |
| public/assets/sprites/rogue-palette.json | Updates rogue palette JSON after normalization. |
| public/assets/sprites/wizard.png | Updates processed wizard sprite asset. |
| public/assets/sprites/wizard_4x.png | Adds/updates 4x upscaled wizard sprite asset. |
| public/assets/sprites/wizard-palette.json | Updates wizard palette JSON after normalization. |
| public/assets/sprites/sorcerer.png | Adds new processed sorcerer sprite asset. |
| public/assets/sprites/sorcerer_4x.png | Adds new 4x upscaled sorcerer sprite asset. |
| public/assets/sprites/sorcerer-palette.json | Adds sorcerer palette JSON. |
| public/assets/sprites/main.palette.json | Adds combined palette index across all sprites. |
| prompts/sprites/class-avatar.prompt.md | Updates sprite prompt notes; adds wizard/sorcerer prompts. |
| package.json | Adds sprite:build-palette and sprite:candidates scripts. |
| .claude/skills/sprite-pipeline/SKILL.md | Updates pipeline upscale guidance to 4x output. |
| .claude/skills/reduce-global-palette/skill.md | Adds new “reduce global palette” workflow doc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const replacements = allMerges | ||
| .map(({ from, into }) => `-fill "${into}" -opaque "${from}"`) | ||
| .map(({ from, into }) => `-fuzz 1% -fill "${into}" -opaque "${from}"`) | ||
| .join(" "); |
There was a problem hiding this comment.
ImageMagick replacements now run with -fuzz 1% for every merge. This changes semantics from exact color replacement to “replace near matches”, which can unintentionally affect additional palette entries in pixel art. If fuzz is only needed in some cases, consider making it configurable (or defaulting to exact match) to keep merges predictable.
| export function TileRow({ items, onPick, columns, onUnpick }: TileRowProps) { | ||
| const interactive = !!onPick; | ||
| const style = columns | ||
| ? { display: "grid", gridTemplateColumns: `repeat(${columns}, 1fr)` } | ||
| : undefined; |
There was a problem hiding this comment.
onPick is optional, but the component derives interactive from it and then calls onPick(...) later. In TypeScript strict mode this typically still types onPick as possibly undefined (because interactive doesn’t narrow the original variable), which can fail compilation. Prefer branching directly on onPick (e.g., if (!onPick) { ... } then the interactive branch has a non-optional onPick) or otherwise use a type guard so the call is type-safe.
| key={item.key} | ||
| type="button" | ||
| aria-label={item.badge ? `${item.label} ${item.badge}` : item.label} | ||
| aria-pressed={item.selected ?? false} | ||
| aria-disabled={item.dimmed} |
There was a problem hiding this comment.
TileRow’s interactive buttons use aria-label derived from the visible label, which makes it hard for callers (e.g. ScorePicker) to provide a more descriptive accessible name without changing the on-screen text. Consider adding an explicit ariaLabel field (or similar) on TileItem and using that (falling back to label) when setting aria-label.
| aria-pressed={item.selected ?? false} | ||
| aria-disabled={item.dimmed} | ||
| className={c( | ||
| styles.tile, | ||
| item.selected && styles.tileSelected, | ||
| item.dimmed && styles.tileDimmed, | ||
| )} | ||
| onClick={() => !item.dimmed && onPick(item.key)} | ||
| onContextMenu={(e) => { |
There was a problem hiding this comment.
Buttons representing dimmed/disabled items set aria-disabled, but they are still enabled native buttons. This keeps them focusable/clickable and requires manual click guards. If dimmed means “disabled”, set the native disabled attribute as well (and you can simplify/remove the !item.dimmed checks).
| const items = sorted.map((v) => ({ | ||
| key: String(v), | ||
| label: String(v), | ||
| selected: v === selectedValue, | ||
| })); |
There was a problem hiding this comment.
Issue #66 asks for a descriptive accessible label like "Select score 15". Right now each TileRow item uses label: String(v), so the button’s accessible name is just the raw number. Update the items you pass to TileRow to include a more descriptive aria-label (this likely requires TileRow to support a separate ariaLabel field as well).
| it("renders a button for each available value", () => { | ||
| render( | ||
| <ScorePicker availableValues={[15, 14, 13]} selectedValue={null} onPick={vi.fn()} />, | ||
| ); | ||
| expect(screen.getByRole("button", { name: "15" })).toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "14" })).toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "13" })).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
These tests assert buttons are discoverable by the name "15"/"14"/etc, but Issue #66’s acceptance criteria expects a more descriptive accessible label (e.g. "Select score 15"). Once ScorePicker provides that label, update the queries/expectations here to match the intended accessible name.
scripts/sprite-merge-candidates.mjs
Outdated
| // find-merge-candidates.mjs | ||
| // Find cross-sprite color pairs that are visually close — candidates for global palette reduction. | ||
| // For each pair, one sprite's color can be replaced with the other's, shrinking the global palette by 1. | ||
| // | ||
| // Usage: | ||
| // node scripts/find-merge-candidates.mjs [--top <N>] [--max-distance <N>] |
There was a problem hiding this comment.
The file header and usage examples refer to find-merge-candidates.mjs, but the actual script is sprite-merge-candidates.mjs (and the package.json script uses that name). This mismatch will mislead users copying the usage comment—please update the header/usage text to the correct filename.
| // find-merge-candidates.mjs | |
| // Find cross-sprite color pairs that are visually close — candidates for global palette reduction. | |
| // For each pair, one sprite's color can be replaced with the other's, shrinking the global palette by 1. | |
| // | |
| // Usage: | |
| // node scripts/find-merge-candidates.mjs [--top <N>] [--max-distance <N>] | |
| // sprite-merge-candidates.mjs | |
| // Find cross-sprite color pairs that are visually close — candidates for global palette reduction. | |
| // For each pair, one sprite's color can be replaced with the other's, shrinking the global palette by 1. | |
| // | |
| // Usage: | |
| // node scripts/sprite-merge-candidates.mjs [--top <N>] [--max-distance <N>] |
| @@ -35,10 +35,6 @@ export function parseExplicitMerges(rawMerges, paletteHexes) { | |||
| console.error(`Error: color ${from} not found in palette`); | |||
| process.exit(1); | |||
| } | |||
There was a problem hiding this comment.
parseExplicitMerges no longer validates that the INTO color exists in the current palette. If INTO is a typo, this will silently introduce a new color; and if --target is also used, the merge-plan math can become inconsistent (merging A→newColor doesn’t reduce the palette size). Consider either reintroducing validation, or making “allow new INTO colors” an explicit opt-in and updating the planning logic to treat missing-INTO as a rename (replace hex) rather than a palette-size reduction.
| } | |
| } | |
| if (!paletteHexes.has(into)) { | |
| console.error(`Error: color ${into} not found in palette`); | |
| process.exit(1); | |
| } |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- tile-row: branch directly on onPick for proper TS narrowing - sprite-merge-candidates: fix stale filename in header comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 40 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const items = sorted.map((v) => ({ | ||
| key: String(v), | ||
| label: String(v), | ||
| selected: v === selectedValue, | ||
| })); | ||
|
|
||
| return ( | ||
| <div className={styles.picker}> | ||
| {sorted.map((value) => ( | ||
| <button | ||
| key={value} | ||
| type="button" | ||
| className={c( | ||
| styles.tile, | ||
| value === selectedValue && styles.tileSelected, | ||
| )} | ||
| onClick={(e) => handleClick(e, value)} | ||
| onKeyDown={(e) => e.stopPropagation()} | ||
| > | ||
| {value} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| <TileRow | ||
| items={items} | ||
| columns={3} | ||
| onPick={(key) => { | ||
| const v = Number(key); | ||
| onPick(v === selectedValue ? null : v); | ||
| }} |
There was a problem hiding this comment.
ScorePicker button accessible names are currently just the numeric value (TileRow uses item.label as aria-label), but Issue #66 requires a descriptive label like "Select score 15" while keeping the visible label as "15". Consider extending TileRow/TileItem to accept a separate ariaLabel (or ariaLabelPrefix) and use that for aria-label here.
| expect(screen.getByRole("button", { name: "15" })).toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "14" })).toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "13" })).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
The tests currently assert buttons have accessible names "15", "14", etc. If ScorePicker is updated to meet Issue #66 (e.g., aria-label "Select score 15"), these assertions should be updated to match the new accessible names so the a11y requirement is enforced by tests.
| <button | ||
| key={item.key} | ||
| type="button" | ||
| aria-label={item.badge ? `${item.label} ${item.badge}` : item.label} | ||
| aria-pressed={item.selected ?? false} | ||
| aria-disabled={item.dimmed} | ||
| className={c( | ||
| styles.tile, | ||
| item.selected && styles.tileSelected, | ||
| item.dimmed && styles.tileDimmed, | ||
| )} | ||
| onClick={() => !item.dimmed && onPick(item.key)} |
There was a problem hiding this comment.
Interactive mode marks dimmed items with aria-disabled but does not set the native disabled attribute. This leaves the button focusable and can confuse assistive tech expectations. Prefer disabled={item.dimmed} (and optionally drop aria-disabled) while keeping the onPick/onUnpick guards.
| .tileRow { | ||
| display: flex; | ||
| gap: var(--space-xs); | ||
| flex-wrap: wrap; | ||
| } |
There was a problem hiding this comment.
TileRow renders a
- in read-only mode, but .tileRow doesn't reset default list styling. Without list-style/margin/padding resets, browsers will add bullets and indentation. Add list-style: none; margin: 0; padding: 0; to .tileRow (or target ul.tileRow) so read-only rows match the intended tile layout.
| export function parseExplicitMerges(rawMerges, paletteHexes) { | ||
| const merges = []; | ||
| for (const raw of rawMerges) { | ||
| const parts = raw.split(":"); | ||
| if (parts.length !== 2) { | ||
| console.error(`Error: --merge must be "FROM:INTO", got: ${raw}`); | ||
| process.exit(1); | ||
| } | ||
| const from = normalize(parts[0]); | ||
| const into = normalize(parts[1]); | ||
| if (!paletteHexes.has(from)) { | ||
| console.error(`Error: color ${from} not found in palette`); | ||
| process.exit(1); | ||
| } | ||
| if (!paletteHexes.has(into)) { | ||
| console.error(`Error: color ${into} not found in palette`); | ||
| process.exit(1); | ||
| } | ||
| merges.push({ from, into, explicit: true }); | ||
| } |
There was a problem hiding this comment.
parseExplicitMerges no longer validates the INTO color at all. If the user passes a malformed hex (wrong length / non-hex chars), the ImageMagick command will fail later with a less clear error. Even if INTO is allowed to be outside the sprite palette (for cross-sprite unification), it would be safer to validate INTO matches a #RRGGBB pattern and error early.
TileRow element (#66)