Add in-app Game Theme Editor (BL-16323) - #8086
Conversation
Introduce a standalone Game Theme Editor web app (src/gameThemeEditor) and wire it into Bloom. The editor lets users view and customize the CSS color variables that drive game themes, with a color picker, a per-color outline, and a contrast checker that flags pairs failing WCAG/UI contrast minimums. - New React/TypeScript app under src/gameThemeEditor with its own build (package.json, tsconfig, host interface) hosted in a draggable/resizable frame. - Host integration from the Games toolbox: ThemeChooser launches the editor and gameThemeEditorHost bridges editor and Bloom. - C# GameThemeEditorApi to read/write theme variables, registered in ProjectContext. - BloomBrowserUI build wiring (vite.config, tsconfig, package.json, yarn.lock).
Brings the in-app Game Theme Editor branch (364 commits behind) up to date with master. Notable resolutions: - Adopted master's yarn->pnpm migration: removed yarn.lock, kept the pnpm-lock.yaml / pnpm-workspace.yaml from master. - package.json: took master's exact version pins over the branch's ^ ranges (react-table 6.11.5, react-tabs 3.2.2, etc.), while preserving react-rnd 10.4.13 which the editor feature (gameThemeEditorHost.ts) depends on and master never carried. - Ran `pnpm install --lockfile-only` so pnpm-lock.yaml includes react-rnd. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
| Filename | Overview |
|---|---|
| src/BloomBrowserUI/bookEdit/toolbox/games/ThemeChooser.tsx | Extends the theme chooser with New/Customize dropdown items, a pencil edit button, and live theme preview colors; uses bare useEffect instead of useMountEffect per AGENTS.md guidance. |
| src/BloomBrowserUI/bookEdit/toolbox/games/gameThemeEditorHost.ts | New host bridge that mounts the editor into the top-level document, manages live CSS-variable preview via injected styles, and implements the IGameThemeEditorHost contract cleanly. |
| src/BloomExe/web/controllers/GameThemeEditorApi.cs | New API controller that reads/writes theme CSS rules (book, collection, factory source); uses Regex.Escape on slug values and documents the invariant clearly. |
| src/gameThemeEditor/src/GameThemeEditorPanel.tsx | Main editor UI with color editing, save/delete actions, and live recoloring; contains a trivially redundant ternary in buttonStyle (both branches produce bloom.blue). |
| src/gameThemeEditor/src/index.tsx | Clean mount/unmount entry point; reuses the React root on re-mount and anchors the Emotion cache to the container document's head, handling cross-document embedding correctly. |
| src/gameThemeEditor/src/contrastUtils.ts | WCAG contrast helpers using a DOM+canvas double-resolution approach to correctly handle color-mix() and translucent colors; logically sound. |
| src/BloomBrowserUI/vite.config.mts | Adds a scoped Vite plugin to re-resolve the game theme editor's bare imports from BloomBrowserUI/node_modules, plus an alias and optimizeDeps entries; cleanly scoped to editor importers only. |
| DistFiles/localization/en/BloomLowPriority.xlf | Adds three new l10n strings for the theme chooser UI; translate=no issue was flagged in a previous review thread. |
| src/content/templates/template books/Games/gamesThemes.less | Intentional contrast fixes for BL-16323: simplifies white-and-orange-on-blue and garden-path themes by removing redundant overrides and alphabetizing declarations. |
| src/gameThemeEditor/src/themeModel.ts | Defines the CSS variable hierarchy, slugify/displayName helpers, and the tree builder; the parent chain correctly mirrors gamesThemes.less .apply-game-theme(). |
Reviews (3): Last reviewed commit: "Revive game theme editor on current mast..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <note>{0} will be replaced with the name of the collection that the user currently has open for editing.</note> | ||
| </trans-unit> | ||
| <trans-unit id="EditTab.Toolbox.Games.NewTheme" translate="no"> | ||
| <source xml:lang="en">New…</source> | ||
| <note>ID: EditTab.Toolbox.Games.NewTheme</note> | ||
| <note>An item in the game "Theme" dropdown in the Games tool of the Edit tab's toolbox. Choosing it starts a brand-new game theme in the theme editor. The trailing ellipsis (…) signals that an editor will open.</note> | ||
| </trans-unit> | ||
| <trans-unit id="EditTab.Toolbox.Games.CustomizeTheme" translate="no"> | ||
| <source xml:lang="en">Customize…</source> | ||
| <note>ID: EditTab.Toolbox.Games.CustomizeTheme</note> | ||
| <note>An item in the game "Theme" dropdown in the Games tool of the Edit tab's toolbox. Choosing it creates a new game theme that starts as a copy of the theme currently applied, then opens it in the theme editor. The trailing ellipsis (…) signals that an editor will open.</note> | ||
| </trans-unit> | ||
| <trans-unit id="EditTab.Toolbox.Games.EditThemeColors" translate="no"> | ||
| <source xml:lang="en">Edit theme colors</source> | ||
| <note>ID: EditTab.Toolbox.Games.EditThemeColors</note> | ||
| <note>Tooltip (hover text) on the pencil/edit button next to the game "Theme" dropdown in the Games tool of the Edit tab's toolbox. Clicking the button opens the game theme editor to change the current theme's colors.</note> | ||
| </trans-unit> | ||
| </body> | ||
| </file> |
There was a problem hiding this comment.
translate="no" on user-visible strings prevents localization
All three new <trans-unit> entries carry translate="no". In XLIFF this attribute tells Crowdin (and any other XLIFF-aware toolchain) that the segment must NOT be translated and should be passed through as-is. "New…", "Customize…", and "Edit theme colors" are real UI strings that end up in the toolbox via useL10n/<Div l10nKey=...>, so Crowdin will silently skip them and non-English users will always see the English text regardless of their locale. The attribute should be removed from all three entries (or, if the intent is deliberately English-only for now, that choice should be documented here rather than signalled via translate="no").
Context Used: AGENTS.md (source)
There was a problem hiding this comment.
[Claude Opus 5] Half agreed, and leaving this open deliberately.
Where this is mistaken: translate="no" on a new entry is this repo's documented convention, not an oversight. .github/skills/xlf-strings/SKILL.md says "Always mark new entries translate=\"no\" unless instructed otherwise", the intent being that a string is withheld from translators until its wording has settled, so nobody pays to translate text that then changes. Devin raised the same point independently and has since withdrawn it for exactly this reason.
Where this is right: the observable consequence you describe is real — until the attribute is removed, non-English users see "New…", "Customize…" and "Edit theme colors" in English next to menu text that is translated.
So the real question is whether the wording is final enough to flip now, which is the developer's call rather than something to decide from convention alone. It is in this run's preflight decision report; this thread stays open until that decision comes back, at which point the outcome gets recorded here.
There was a problem hiding this comment.
Thanks for the correction — that's fair, and the distinction you've drawn is exactly right.
My comment conflated two separate questions: (1) is translate="no" correct per convention? and (2) is the wording settled enough to drop it? The first has a clear answer (yes, per the documented convention), and I shouldn't have flagged it as a defect. The second is genuinely open and is the developer's call, so keeping this thread open pending that decision makes sense.
For what it's worth, I already have a rule (from a previous correction on this same pattern) that says not to flag translate="no" on new XLF entries as a defect. The fact that I fired anyway suggests the rule needs tightening — the right framing for future reviews is: new entries with translate="no" are correct; worth a note only if the strings look stable enough to flip now, and even then as a nudge rather than a defect.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
e053636 to
42f492d
Compare
…es (BL-16323) Brings the in-app game theme editor branch back to life on top of a freshly merged master, and finishes the game-theme contrast and localization work. Build: the editor is a self-contained project whose source lives in a sibling directory (src/gameThemeEditor/src), outside BloomBrowserUI. After master's yarn->pnpm migration and its new tsgo type-check gate, two build steps could no longer resolve the editor's bare dependency imports (@emotion/react, @emotion/cache, react-dom, react-rnd) by walking up out of that folder: - Type check: added tsconfig "paths" entries mapping those specifiers into BloomBrowserUI/node_modules, mirroring the existing "react"/"gameThemeEditor" mappings. - Production build (vite / Rollup): added a small, build-only Vite plugin that re-resolves those ids from BloomBrowserUI's root, scoped to importers inside the editor directory so the rest of the app is untouched and react stays deduped. This is the build-time counterpart to the dev server's existing optimizeDeps.include handling. Localization: added the three missing Games-tool theme-chooser strings to BloomLowPriority.xlf (NewTheme "New…", CustomizeTheme "Customize…", EditThemeColors "Edit theme colors"), each translate="no" with a translator context note, and routed the edit-button tooltip through useL10n. Themes: further contrast fixes to the game themes (gamesThemes.less). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
42f492d to
8624b7e
Compare
| /// <summary>Upsert the theme's rule into the file at <paramref name="path"/>, and—when the | ||
| /// theme was renamed—remove the rule for its previous slug so a rename does not leave a | ||
| /// duplicate behind.</summary> | ||
| private static void WriteThemeToFile(string path, ThemeDto theme) |
There was a problem hiding this comment.
[Devin] Investigate: Theme rename removes the old rule only from the file it's saving into
When a theme is renamed, the old CSS rule is removed only from the stylesheet the renamed theme is being written to. If the original rule lived in a different location (e.g. defined at collection level but now saved to the book, or vice-versa), the old rule is never removed and can linger as an orphaned/duplicate theme rule.
Pre-existing editor logic; left open for developer assessment (see preflight decision report).
|
[Claude Opus 4.8] Consulted Devin on 2026-07-20 (22:36 UTC) up to commit 1 Bug — "New theme-menu labels will never appear in other languages" ( 2 Investigate flags:
7 Informational flags (not posted — low signal): pencil button clickable while editor open; ColorPicker outside-click effect re-subscribes each render; contrast probe/cache never invalidates; C# theme-block rewrite relies on the flat-declaration invariant (verified to hold); white-and-orange-on-blue header background semantics changed (intentional contrast fix); stale mount-location comments; unguarded localStorage write. CI ( |
…X (BL-16323) Two preflight fixes to the game theme editor branch: pnpm-lock.yaml had been rewritten in raw pnpm style (2-space indent, single quotes) instead of the repo's prettier-formatted style (4-space, double quotes). Nothing about the dependency graph actually changed -- the file is not in .prettierignore, so prettier owns it -- but the reformatting inflated the diff to ~30,400 lines and would have made merging master (151 commits ahead) needlessly painful. Running prettier over it reduces the change to the 58 lines that genuinely add react-rnd and its transitive deps (react-draggable, re-resizable, tslib). tsconfig.json was missing path mappings for @emotion/react/jsx-runtime and jsx-dev-runtime. The editor sets jsxImportSource to @emotion/react, so every .tsx file under ../gameThemeEditor resolves its JSX factory through that subpath; from the sibling directory the lookup failed and all eight components reported TS2875. Because TS2875 is not one of the blunder-class codes the typecheck gate fails on, this went unnoticed and the editor's JSX was silently going unchecked. With the mappings in place the editor type-checks clean, so this closes the hole without hiding any existing errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UpsertThemeRule passed the generated CSS rule to Regex.Replace as a replacement
string. .NET interprets "$" in a replacement as a substitution token ($1, $&,
${name}), so any theme variable whose value contained a "$" would have been
silently mangled when overwriting an existing rule -- while appending a brand-new
rule, which does not go through Regex.Replace, would have written it correctly.
Substituting via a MatchEvaluator inserts the rule verbatim. Slugs are unaffected
either way (slugify() strips them to [\w-]).
GameThemeEditorPanel decided whether to auto-focus and select the name field by
testing displayName.startsWith("Untitled"). The panel already computes isNewTheme
a hundred lines earlier from the host's explicit "new" signal
(getNewThemeName() !== null), so the string sniff was both redundant and wrong at
the edges: it would have selected the name of an existing user theme actually
called something like "Untitled sketch", and it silently coupled focus behavior to
the English wording of the suggested name, so localizing "Untitled Theme N" later
would have broken it with no compile-time signal. Use isNewTheme.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # DistFiles/localization/en/BloomLowPriority.xlf
…on/cache (BL-16323) Two findings from Devin's review of 5aee1b2. ThemeChooser's two mount-only effects (the editor open-state subscription and the developer-capability fetch) were written as bare useEffect(..., []). The front-end AGENTS.md asks for the useMountEffect helper instead, which exists precisely so the single justified exhaustive-deps suppression lives in one place rather than being rewritten per call site. No runtime change; the third effect in this file has real dependencies and is left alone. @emotion/cache was imported by the editor (src/gameThemeEditor/src/index.tsx) but declared nowhere: BloomBrowserUI depends only on @emotion/core, /react and /styled, so the package was reaching it as a transitive dependency of @emotion/react. That resolves today, and the production bundle builds, but pnpm's isolated node_modules layout only guarantees direct dependencies at the project root, so this was working by accident of the current layout rather than by declaration. Added it as a direct dependency pinned to 11.10.5 -- the version already resolved in the tree -- so nothing else moves. The lockfile is re-prettified after the install: pnpm rewrites it in its own 2-space style, which is what produced the ~30,000-line diff cleaned up in 7716b7b. The net lockfile change here is 41 lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| @@ -65,36 +65,19 @@ | |||
| } | |||
|
|
|||
| .bloom-page.game-theme-white-and-orange-on-blue { | |||
There was a problem hiding this comment.
[Devin] Investigate: white-and-orange theme drops the control-button-bg contrast override
Rewriting .bloom-page.game-theme-white-and-orange-on-blue removed two colour overrides that had been added for this very ticket, along with the comments explaining them:
--game-control-button-bg-color: #ffce8f;--game-checkbox-outline-color: white;
The same --game-control-button-bg-color override (#ffdccf) was also dropped from .game-theme-coral-reef.
Why this matters. With the override gone, the value falls back through the defaults in this file — --game-control-button-bg-color: var(--game-primary-bg-color) (line 43) and --game-primary-bg-color: var(--game-primary-color) (line 20) — so it lands on the theme's #ffb453. The comment that was deleted alongside it recorded that this orange is 2.81:1 against the blue page, just under the 3:1 minimum for UI components, which is exactly why the brighter #ffce8f (3.4:1) was introduced. --game-control-button-bg-color is used as a real background (Games.less:586), so this is visible, not cosmetic. The deleted checkbox-outline comment likewise recorded a move from a nearly-invisible grey (1.32:1) to white (4.95:1).
I verified that neither derivation was changed by this branch, so the fallback really does resolve to the lower-contrast colour.
This looks like an unintended side effect of round-tripping the factory themes through the new editor: the output is alphabetised, comments are stripped, and dropRedundantColors removes values it considers derivable — but these two were deliberately not the derived value.
Flagging rather than fixing, because re-tuning these themes may well have been intentional. Raised for a decision in the preflight report.
|
[Claude Opus 5] Consulted Devin on 2026-07-27 (17:15 UTC) up to commit That review raised 2 bugs, 4 investigate flags and 11 informational items. Outcomes:
The 11 informational items were reviewed and none warranted a change. CI is green. |
…6323) Two more findings from Devin's review of d979ffc. mount() created a fresh Emotion cache on every call, anchored to the page document's <head>, and unmount() only tore down the React root. Emotion's <style> elements live in that head rather than inside the container, so removing the container (which the host does on close) left them behind -- and because the host recreates the container on each open, every open/close cycle added another full set of style nodes to the page. The cache is now kept alongside the root and reused for re-renders instead of being recreated, and unmount() calls cache.sheet.flush(), Emotion's own API for removing the elements it inserted. The default save target was chosen with a stacked ternary. The root AGENTS.md explicitly rules that out ("Avoid stacking/nesting ternary operators ... Use an if/else-if chain (or a switch) instead"), so it is now an if/else-if chain. No behavior change. Verified with typecheck, the full Vitest suite, and an isolated production Vite build (agent-vite), since the Emotion change affects real bundling rather than just types. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…6323) Devin promoted this to a bug on re-review of fae350e, and it holds up. save() guards against writing over a different theme with `theme.slug !== startingSlug && host.themeExists(theme.slug)`. But startingSlug is whatever theme the page was displaying when the editor opened, and for "New…" and "Customize…" the host deliberately switches the page to the base theme first. So if the user names the new theme after the one it was based on, the first half of the condition is false, the existence check never runs, and Save writes that slug with no warning. Where the base is a custom book/collection theme, that overwrites its colors outright; where it is a factory theme, it silently creates an override the user did not ask for. Comparing against renameFromSlug instead gives exactly the intended semantics: it is "" for a new theme, so any existing name now trips the guard; it equals the theme's own slug when editing in place, so saving over yourself is still allowed; and it is the old slug when renaming, so the new name is still checked. The guard already had the right intent -- it was just comparing against the wrong thing. Likelihood is low, since new themes are pre-seeded with "Untitled Theme N" and the user has to retype the base name, but the failure is silent data loss, so it is worth the one-word fix. Also updates two PAPERCUTS entries with what this run cost: - the agent-dotnet environmental-failure baseline is 19 now, not 9, and needs the trx logger to enumerate; recording it by test-class pattern rather than count. - the pnpm-lock format drift is prettier's formatting, not an old pnpm style, so `prettier --write pnpm-lock.yaml` after an install fixes it in one step -- no hand-patching of hashes as that entry previously advised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| const renameFromSlug = isNewTheme ? "" : startingSlug; | ||
| // Default the Save button to wherever the theme already lives: a collection theme defaults to | ||
| // "Save to Collection"; a factory theme to source; otherwise (book/new) to "Save to Book". | ||
| const themeSource = isNewTheme ? "none" : host.getThemeSource(); |
There was a problem hiding this comment.
[Devin] Investigate: Collection theme has no primary Save button while Save-to-Collection is disabled
defaultTarget can still evaluate to "collection", but the Save to Collection button is commented out as "temporarily disabled". For a theme whose source is the collection, that leaves no highlighted default action at all: Save to Book renders with the non-primary outline variant, because defaultTarget === "book" is false.
Two consequences worth confirming are intended for now:
- No primary action. A user editing a collection theme sees only outline buttons, so nothing indicates the expected action.
- Silent duplication. Saving to the book writes the theme there while the collection copy stays on disk, so the same slug ends up defined in both places. (Related to, but distinct from, the rename-orphan thread above.)
I confirmed the button is commented out and that defaultTarget is still allowed to be "collection", so both follow. Not a crash, and not fixed here — the right resolution depends on why collection-save is disabled, which is a call for the developer. Raised in the preflight decision report.
|
[Claude Opus 5] Consulted Devin again on 2026-07-27 (18:03 UTC) up to commit Devin marks all 5 bugs it raised across this run as fixed, and reports 0 unfixed bugs at this commit. Over the three reviews it raised 5 bugs and 7 investigate flags; 15 informational items were reviewed and none warranted a change. Fixed and resolved this run: the Assessed as not an issue and resolved: the cross-realm React root (the probe span is a cached singleton, and the mount target is deliberate and documented). Two flags were stale — they cite a Three threads are deliberately left open for the developer, all in the preflight decision report:
plus Greptile's P1 on CI is green. The bot fix/re-review loop reached this skill's 4-cycle cap, so the one new flag at this commit was posted rather than fixed. |
|
[Claude Opus 5] Superseded by #8118 — closed by a branch rename, not abandoned. This work was sharing BL-16323 with the game-theme colour tweaks that already shipped separately in #7985. It now has its own card, BL-16618, and the branch was renamed The review history here is still the record: the Devin consultation log and the mirrored finding threads live on this PR. #8118 links back here and lists the four findings that were still open, so nothing is lost — but new review activity should go to #8118. One of those four (the factory-theme contrast regression in |
1 participant
In-app Game Theme Editor
Adds a self-contained editor for Bloom game themes (named sets of CSS custom properties that style drag-activity game pages), reachable from the Games tool's theme chooser in the Edit-tab toolbox.
What's here
src/gameThemeEditor/— a self-contained React/TypeScript editor project (color pickers, contrast checker, draggable/resizable floating panel). It depends on nothing in BloomBrowserUI; the only coupling is theIGameThemeEditorHostcontract plusmount()/unmount().bookEdit/toolbox/games/gameThemeEditorHost.tsmounts the editor over the live page for real-time recoloring;ThemeChooser.tsxgains New…, Customize…, and an edit (pencil) button that open it.GameThemeEditorApi— reads/writes theme variables (collection + developer-only factory source).gamesThemes.less(BL-16323).This session's follow-up work (on top of a fresh master merge)
paths(type check) and a small, build-only, editor-scoped Vite plugin (production build) — the build-time counterpart to the dev server's existingoptimizeDepshandling.BloomLowPriority.xlf(NewTheme,CustomizeTheme,EditThemeColors) and routed the edit-button tooltip throughuseL10n.Notes for reviewers
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16323
Devin review
This change is