Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions DistFiles/localization/en/BloomLowPriority.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,21 @@
<note>ID: CollectionTab.BookMenu.MoveToCurrentCollection</note>
<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">
Comment thread
hatton marked this conversation as resolved.
<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>
Comment on lines 354 to 382

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</xliff>
247 changes: 217 additions & 30 deletions src/BloomBrowserUI/bookEdit/toolbox/games/ThemeChooser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,27 @@ import {
toolboxMenuPopupTheme,
} from "../../../bloomMaterialUITheme";
import MenuItem from "@mui/material/MenuItem";
import Divider from "@mui/material/Divider";
import { Div } from "../../../react_components/l10nComponents";
import { useL10n } from "../../../react_components/l10nHooks";
import { InfoIconUrl } from "../../../react_components/icons/InfoIconUrl";
import BloomSelect from "../../../react_components/bloomSelect";
import EditIcon from "@mui/icons-material/Edit";
import IconButton from "@mui/material/IconButton";
import { getAsync } from "../../../utils/bloomApi";
import {
showGameThemeEditor,
showNewGameThemeEditor,
showCustomizeGameThemeEditor,
isGameThemeEditorOpen,
subscribeGameThemeEditorOpen,
isFactoryThemeSlug,
resolveThemeHeaderColors,
} from "./gameThemeEditorHost";

// Sentinel values for the "New…" and "Customize…" items in the theme dropdown (not real themes).
const kNewThemeValue = "__new_game_theme__";
const kCustomizeThemeValue = "__customize_game_theme__";

const getPage = () => {
const pageBody = ToolBox.getPage();
Expand Down Expand Up @@ -44,8 +62,122 @@ export const ThemeChooser: React.FunctionComponent<{
// gameThemePrefix, or "default" if there is none (but migration code and this tool makes sure
// that game pages always do).
const [currentTheme, setCurrentTheme] = useState("");
// While the floating theme editor is open it owns theme changes, so we disable the
// dropdown to avoid switching themes out from under it. The editor can be closed from
// its own controls, so we track its open state via the host's subscription.
const [editorOpen, setEditorOpen] = useState(isGameThemeEditorOpen());
// Bumped when the editor closes, so we re-scan the available themes: a save may have added
// or renamed one (a rename removes the old name's rule from the page).
const [themesRefreshKey, setThemesRefreshKey] = useState(0);
useEffect(() => {
setEditorOpen(isGameThemeEditorOpen());
return subscribeGameThemeEditorOpen(() => {
const open = isGameThemeEditorOpen();
setEditorOpen(open);
if (!open) setThemesRefreshKey((k) => k + 1);
});
}, []);
// Whether Bloom is running from source (developers can edit factory themes). Non-developers
// don't get the edit button on factory themes, since they can't change them.
const [isDeveloper, setIsDeveloper] = useState(false);
useEffect(() => {
getAsync("gameThemeEditor/canSaveToFactorySource").then((result) => {
setIsDeveloper(!!(result && (result as { data?: boolean }).data));
});
}, []);
// Tooltip for the edit (pencil) button that opens the game theme editor.
const editThemeColorsTitle = useL10n(
"Edit theme colors",
"EditTab.Toolbox.Games.EditThemeColors",
);
const currentThemeIsFactory = React.useMemo(
() => isFactoryThemeSlug(currentTheme),
// Re-evaluate when the page changes too, since the available stylesheets can change.
// eslint-disable-next-line react-hooks/exhaustive-deps
[currentTheme, props.pageGeneration],
);
// Each dropdown item previews its theme by using that theme's header colors (the menu
// items only render while the dropdown is open, so this styling shows only then).
const headerColors = React.useMemo(() => {
const map: Record<string, { bg: string; color: string }> = {};
for (const theme of themes)
map[theme] = resolveThemeHeaderColors(theme);
return map;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [themes, props.pageGeneration]);
// Which themes are factory (built-in) vs custom. We only run the localization system on
// factory names; custom theme names are user-defined and not in our localization system, so
// looking them up just clutters the screen with "untranslated" warnings.
const themeIsFactory = React.useMemo(() => {
const map: Record<string, boolean> = {};
for (const theme of themes) map[theme] = isFactoryThemeSlug(theme);
return map;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [themes, props.pageGeneration]);
// The dropdown lists factory themes first, then a divider and "New…", then the user's
// custom themes. Partition the (already sorted) themes accordingly.
const factoryThemes = React.useMemo(
() => themes.filter((theme) => themeIsFactory[theme]),
[themes, themeIsFactory],
);
const customThemes = React.useMemo(
() => themes.filter((theme) => !themeIsFactory[theme]),
[themes, themeIsFactory],
);
// Render one theme as a dropdown item, previewing it with its own header colors.
const renderThemeItem = (theme: string) => (
<MenuItem
value={theme}
key={theme}
disabled={false}
// Inline style so the theme's header colors win over MUI's hover/selected background.
style={{
backgroundColor: headerColors[theme]?.bg || undefined,
color: headerColors[theme]?.color || undefined,
}}
>
{themeIsFactory[theme] ? (
<Div
l10nKey={`EditTab.Toolbox.Games.Themes.${theme}`}
// Suppress the "untranslated" warning: dev-added source
// themes won't have localization entries either.
temporarilyDisableI18nWarning={true}
>
{isMissingTheme(theme)
? `(Missing) ${getThemeLabel(theme)}`
: getThemeLabel(theme)}
</Div>
) : (
// Custom theme: render the name as-is; never run localization.
<span>
{isMissingTheme(theme)
? `(Missing) ${getThemeLabel(theme)}`
: getThemeLabel(theme)}
</span>
)}
</MenuItem>
);
// A fresh, unused "Untitled Theme N" name for a brand-new or customized theme.
const nextUntitledName = () => {
let n = 1;
while (themes.includes(`untitled-theme-${n}`)) n++;
return `Untitled Theme ${n}`;
};
// "New…": a brand-new theme based on the default factory theme (Blue On White).
const handleNewTheme = () => showNewGameThemeEditor(nextUntitledName());
// "Customize…": a new theme that starts as a copy of the theme currently applied.
const handleCustomizeTheme = () =>
showCustomizeGameThemeEditor(nextUntitledName());
const handleChooseTheme = (event) => {
const newTheme = event.target.value;
if (newTheme === kNewThemeValue) {
handleNewTheme();
return;
}
if (newTheme === kCustomizeThemeValue) {
handleCustomizeTheme();
return;
}
if (newTheme === currentTheme) {
return;
}
Expand Down Expand Up @@ -143,8 +275,11 @@ export const ThemeChooser: React.FunctionComponent<{
);

// We don't need to run again if currentTheme changes, since it can only change to something
// that's already in the list (except just possibly when pageGeneration changes).
}, [props.pageGeneration]);
// that's already in the list (except just possibly when pageGeneration changes). We also
// re-run when the editor closes (themesRefreshKey), since a save/rename may have changed
// the set of themes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.pageGeneration, themesRefreshKey]);
return (
<ThemeProvider theme={toolboxMenuPopupTheme}>
<div
Expand Down Expand Up @@ -193,39 +328,91 @@ export const ThemeChooser: React.FunctionComponent<{
}}
/>
</div>
<BloomSelect
variant="standard"
value={currentTheme}
onChange={(event) => {
handleChooseTheme(event);
}}
inputProps={{
name: "style",
id: "game-theme-dropdown",
}}
<div
css={css`
svg.MuiSvgIcon-root {
color: white !important;
}
ul {
background-color: ${kOptionPanelBackgroundColor} !important;
}
fieldset {
border-color: rgba(255, 255, 255, 0.5) !important;
}
display: flex;
align-items: center;
`}
size="small"
>
{themes.map((theme) => (
<MenuItem value={theme} key={theme} disabled={false}>
<Div l10nKey={`EditTab.Toolbox.Games.Themes.${theme}`}>
{isMissingTheme(theme)
? `(Missing) ${getThemeLabel(theme)}`
: getThemeLabel(theme)}
<BloomSelect
variant="standard"
value={currentTheme}
disabled={editorOpen}
onChange={(event) => {
handleChooseTheme(event);
}}
inputProps={{
name: "style",
id: "game-theme-dropdown",
}}
css={css`
flex: 1;
// Allow the select to shrink below its content width so a long theme
// name truncates (with an ellipsis) instead of pushing the edit button
// out of the narrow toolbox.
min-width: 0;
.MuiSelect-select {
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
}
// While the editor is open the dropdown is disabled (so themes can't be
// switched out from under it), but MUI's default disabled styling greys the
// text to near-invisible on the dark toolbox. Keep it readable.
.MuiSelect-select.Mui-disabled {
color: white !important;
-webkit-text-fill-color: white !important;
}
svg.MuiSvgIcon-root {
color: white !important;
}
ul {
background-color: ${kOptionPanelBackgroundColor} !important;
}
fieldset {
border-color: rgba(255, 255, 255, 0.5) !important;
}
`}
size="small"
>
{/* Factory themes first, then a divider, then "New…", then custom themes. */}
{factoryThemes.map(renderThemeItem)}
<Divider />
{/* Not themes: "New…" starts from Blue On White; "Customize…" copies the
current theme. Both open the editor on a new, unsaved theme. */}
<MenuItem value={kNewThemeValue} key={kNewThemeValue}>
<Div l10nKey="EditTab.Toolbox.Games.NewTheme">New…</Div>
</MenuItem>
<MenuItem
value={kCustomizeThemeValue}
key={kCustomizeThemeValue}
>
<Div l10nKey="EditTab.Toolbox.Games.CustomizeTheme">
Customize…
</Div>
</MenuItem>
))}
</BloomSelect>
{customThemes.map(renderThemeItem)}
</BloomSelect>
{/* Non-developers can't change factory themes, so don't offer the edit button
for them on a factory theme (they can still use "New…" to make a copy). */}
{(isDeveloper || !currentThemeIsFactory) && (
<IconButton
id="game-theme-edit-button"
size="small"
// Opens the floating game theme editor over the live page so it can recolor
// the real game in real time. Mounted from here (toolbox) into the page document.
onClick={() => showGameThemeEditor()}
title={editThemeColorsTitle}
css={css`
color: white !important;
margin-left: 4px;
flex-shrink: 0;
`}
>
<EditIcon fontSize="small" />
</IconButton>
)}
</div>
</ThemeProvider>
);
};
Loading