Add Settings UI improvements with Display tab and theme modes #2724
Conversation
- Add Display tab with Theme Mode selector (light/dark/system) - Add Territory Skins component (cosmetics disabled placeholder) - Add Color Palette component (cosmetics disabled placeholder) - Add collapsible SettingGroup component for organizing settings - Group existing settings under Interface, Graphics, Controls, Privacy - Add light theme support for modals with CSS variables - Sync DarkModeButton with new theme mode system - Add MutationObserver to Modal for real-time theme updates - Persist theme preference and apply on page load 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughCentralizes theme into a tri-state ThemeMode (light|dark|system), applies theme at startup, adds theme-aware modal styling, introduces grouped/settings UI (Display tab) with new Lit components, updates styles and localization, and adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ThemeUI as SettingThemeMode
participant UserSettings
participant EventBus as window (events)
participant DarkToggle as DarkModeButton
participant Modal
User->>ThemeUI: Click "Dark"/"Light"/"System"
ThemeUI->>ThemeUI: persist selection (localStorage)
ThemeUI->>UserSettings: setThemeMode(mode)
UserSettings->>UserSettings: applyTheme()
UserSettings->>EventBus: dispatch "dark-mode-changed"
EventBus->>DarkToggle: update UI (isDarkModeActive)
EventBus->>Modal: MutationObserver/updateTheme -> re-render
ThemeUI->>ThemeUI: update selection visuals
sequenceDiagram
autonumber
participant Page as Page Load
participant Main
participant UserSettings
participant Document
Page->>Main: App init
Main->>UserSettings: applyTheme()
UserSettings->>UserSettings: themeMode() (load/migrate)
UserSettings->>UserSettings: isDarkModeActive()
UserSettings->>Document: Add/Remove "dark" class
Document->>Document: CSS variables & styles take effect
Main->>Main: Continue startup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-06-02T14:27:37.609ZApplied to files:
📚 Learning: 2025-08-27T08:12:19.610ZApplied to files:
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
src/client/components/baseComponents/Modal.ts (1)
119-170: Good use of CSS variables for theming.The CSS variable approach for light/dark themes is clean and makes child components easy to style. Both theme variants define matching variable sets, which keeps the styling consistent.
One small note: the dark theme variables are defined twice (lines 53-62 for shadows/borders, then again lines 156-170 for the full set). This is fine since CSS cascades, but you could consolidate them in one block for clarity.
src/client/components/baseComponents/setting/SettingGroup.ts (1)
68-80: Avoid!importantwhen possible.The use of
!importanton multiple display properties (lines 69, 73-74, 79) can make future styling changes difficult. Since this is a scoped Lit component, consider using more specific selectors or restructuring to avoid the override war.src/client/components/baseComponents/setting/SettingThemeMode.ts (1)
136-163: Consider keyboard accessibility for theme options.The theme options use
<div>with@clickhandlers. For keyboard users, consider addingtabindex="0"and@keydownhandlers (Enter/Space to select), or switch to<button>elements.Example using buttons for better accessibility
- <div - class="theme-option ${this.selectedMode === "light" - ? "selected" - : ""}" - @click=${() => this.selectMode("light")} - > + <button + type="button" + class="theme-option ${this.selectedMode === "light" + ? "selected" + : ""}" + @click=${() => this.selectMode("light")} + > <span class="theme-icon">☀️</span> <span class="theme-label">Light</span> - </div> + </button>Apply similar changes to the other two options.
src/client/components/baseComponents/setting/SettingTerritorySkins.ts (2)
297-309: saveSkin always writes customSkinUrl even when null.When
customSkinUrlis null, the old custom URL remains in localStorage. If you want to clear it when selecting a predefined skin, explicitly remove it.Proposed fix
private saveSkin() { localStorage.setItem("settings.territorySkin", this.selectedSkinId); if (this.customSkinUrl) { localStorage.setItem("settings.territorySkinCustom", this.customSkinUrl); + } else { + localStorage.removeItem("settings.territorySkinCustom"); }
347-350: handleSaveClick is redundant.The "Save" button just calls
saveSkin(), butselectSkin()already callssaveSkin()after each selection. The explicit Save button may confuse users—either remove it or change the flow so selections are not auto-saved.src/client/components/baseComponents/setting/SettingColorPalette.ts (5)
64-64: Clarify disabled state behavior for persistence and events.The component persists colors to localStorage and emits change events even when
disabled={true}. Since the "Cosmetics Module Required" badge suggests this feature isn't available, consider whether persistence and event emission should be blocked in disabled state. Currently, users can interact with a disabled feature that saves data but may not actually apply anywhere.If the disabled state is purely visual (to show what's coming), the current behavior is fine. However, if users shouldn't be able to configure cosmetics without the module, add guards:
🔎 Suggested guards for disabled state
private saveColors() { + if (this.disabled) return; localStorage.setItem("settings.colorPalette", JSON.stringify(this.colors)); this.dispatchEvent( new CustomEvent("change", {Also applies to: 282-314
329-357: Add validation for hex color input format.The
hexToHslmethod assumes the input is always a valid 7-character hex string (#RRGGBB). If localStorage contains a malformed color (short form#RGB, missing#, or corrupted data),parseIntwill returnNaNand produce invalid HSL values.🔎 Add validation to prevent NaN values
private hexToHsl(hex: string): { h: number; s: number; l: number } { + // Validate hex format + if (!/^#[0-9A-Fa-f]{6}$/.test(hex)) { + console.warn(`Invalid hex color: ${hex}, using fallback`); + hex = "#4CAF50"; + } const r = parseInt(hex.slice(1, 3), 16) / 255;
433-448: Consider real-time hex input validation.The hex input only validates on
@change(line 492), which fires when the input loses focus. Users might type a valid hex but not see the update until they click elsewhere. Consider adding@inputor@blurfor better UX, or show visual feedback while typing.🔎 Add blur event for better UX
<input type="text" class="color-input__text" .value=${currentColor} - @change=${this.handleHexInput} + @blur=${this.handleHexInput} ?disabled=${this.disabled} />This triggers the update as soon as the user leaves the field, which feels more responsive.
Also applies to: 488-494
403-425: Consider adding drag support for color picker.The color preview and hue slider only respond to clicks, requiring users to click repeatedly to adjust colors. Standard color pickers support dragging for smoother interaction.
You could add
mousedown+mousemovehandlers with pointer capture for a better UX:private handlePointerDown(e: PointerEvent, handler: (e: PointerEvent) => void) { if (this.disabled) return; const target = e.currentTarget as HTMLElement; target.setPointerCapture(e.pointerId); handler(e); const onMove = (e: PointerEvent) => handler(e); const onUp = (e: PointerEvent) => { target.releasePointerCapture(e.pointerId); target.removeEventListener('pointermove', onMove); target.removeEventListener('pointerup', onUp); }; target.addEventListener('pointermove', onMove); target.addEventListener('pointerup', onUp); }Then use
@pointerdowninstead of@clickin your template.
281-303: Handle partial color data from localStorage.If localStorage contains an older, incomplete color set (e.g., user had 5 swatches but now there are 8), missing colors won't be initialized. This could cause
undefinedvalues in thecolorsrecord when rendering new swatches.🔎 Merge saved colors with defaults
private loadColors() { const saved = localStorage.getItem("settings.colorPalette"); + const defaults = this.initializeDefaultColors(); if (saved) { try { - this.colors = JSON.parse(saved); + this.colors = { ...defaults, ...JSON.parse(saved) }; } catch { - this.initializeDefaultColors(); + this.colors = defaults; } } else { - this.initializeDefaultColors(); + this.colors = defaults; } this.updatePickerFromSelectedColor(); } private initializeDefaultColors() { - this.colors = DEFAULT_SWATCHES.reduce( + return DEFAULT_SWATCHES.reduce( (acc, swatch) => {src/core/game/UserSettings.ts (2)
70-80: Consider persisting migrated theme mode.The migration from legacy
darkModetothemeModehappens on every read but doesn't write the migrated value. This means the legacy check runs repeatedly. Consider storing the migrated value:🔎 Persist migrated value
themeMode(): ThemeMode { const value = localStorage.getItem("settings.themeMode"); if (value === "light" || value === "dark" || value === "system") { return value; } // Migrate from legacy darkMode setting - if (this.darkMode()) { - return "dark"; + const migrated = this.darkMode() ? "dark" : "system"; + localStorage.setItem("settings.themeMode", migrated); + return migrated; - } - return "system"; }This runs the migration once per user, then uses the cached value.
87-104: System theme changes won't update automatically.When
themeModeis"system", the app checksprefers-color-schemeonce but doesn't listen for changes. If the user switches their OS theme while the app is open, the UI won't update until they reload.Consider adding a media query listener when in system mode:
🔎 Listen to system theme changes
In your initialization code (e.g.,
Main.ts), after callingapplyTheme():const settings = new UserSettings(); settings.applyTheme(); // Listen for system theme changes const mediaQuery = window.matchMedia("(prefers-color-scheme: dark)"); mediaQuery.addEventListener("change", () => { if (settings.themeMode() === "system") { settings.applyTheme(); } });This keeps the UI in sync with OS theme changes when using system mode.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignoreresources/lang/en.jsonsrc/client/DarkModeButton.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/Modal.tssrc/client/components/baseComponents/setting/SettingColorPalette.tssrc/client/components/baseComponents/setting/SettingGroup.tssrc/client/components/baseComponents/setting/SettingTerritorySkins.tssrc/client/components/baseComponents/setting/SettingThemeMode.tssrc/client/styles/components/setting.csssrc/core/game/UserSettings.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/core/game/UserSettings.tssrc/client/UserSettingModal.ts
📚 Learning: 2025-06-07T19:47:34.514Z
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/styles/components/setting.css:9-22
Timestamp: 2025-06-07T19:47:34.514Z
Learning: In src/client/styles/components/setting.css, the padding difference between .setting-item and .setting-item-group is intentional - .setting-item-group doesn't need padding for the specific grouped setting styling being implemented.
Applied to files:
src/client/styles/components/setting.csssrc/client/components/baseComponents/setting/SettingGroup.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/components/baseComponents/setting/SettingColorPalette.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.tssrc/client/components/baseComponents/Modal.tssrc/client/UserSettingModal.ts
🧬 Code graph analysis (2)
src/client/UserSettingModal.ts (1)
src/core/game/UserSettings.ts (2)
UserSettings(8-260)ThemeMode(6-6)
src/client/components/baseComponents/setting/SettingThemeMode.ts (1)
src/core/game/UserSettings.ts (2)
ThemeMode(6-6)darkMode(66-68)
🔇 Additional comments (27)
.gitignore (1)
12-12: Approved — consistent with existing tool directory ignores.The addition of
.claude/follows the established pattern (e.g.,.clinic/on line 11) and is appropriately placed alongside other tool-specific ignores.src/client/components/baseComponents/Modal.ts (2)
16-30: Clean lifecycle management for theme observation.The MutationObserver pattern for tracking theme changes is well-implemented. Proper setup in
connectedCallbackand cleanup indisconnectedCallbackprevents memory leaks.
184-190: Class computation is clear and readable.Using array filter and join for building class strings is idiomatic and easy to follow.
src/client/components/baseComponents/setting/SettingGroup.ts (2)
96-107: Good defensive coding for localStorage access.The try-catch around JSON parsing protects against corrupted data. The early return when
groupIdis empty prevents saving with an invalid key.
128-152: Clear render logic with proper state handling.The toggle interaction and class computation are straightforward. The use of a slot for content projection is the right pattern for a wrapper component.
src/client/DarkModeButton.ts (2)
8-8: Good migration to centralized theme API.Using
isDarkModeActive()instead of the rawdarkMode()getter aligns this component with the new tri-state theme system.
29-32: Verify toggleDarkMode behavior with system theme.When
themeModeis"system", callingtoggleDarkMode()will toggle to an explicit mode. Confirm this is the intended behavior—users might expect the toggle to switch between light/dark while staying in "system" mode.src/client/Main.ts (1)
347-348: Clean simplification of theme initialization.Replacing manual DOM class manipulation with
applyTheme()centralizes the theme logic. This makes startup behavior consistent with runtime theme changes.src/client/components/baseComponents/setting/SettingThemeMode.ts (2)
102-111: Legacy migration handles existing users well.The fallback from
settings.themeModetosettings.darkModeensures existing users get a sensible default. MappingdarkMode: true→"dark"andfalse→"system"is reasonable.
117-126: Component dispatches event but does not persist.
selectModeupdates local state and emits an event, but does not write tolocalStorage. The parent (UserSettingModal.handleThemeModeChange) callsthis.userSettings.setThemeMode(mode)which should persist. This is the right separation of concerns—the component is a pure UI control.src/client/components/baseComponents/setting/SettingTerritorySkins.ts (1)
88-93: Component structure and disabled state look good.The disabled overlay pattern matches the SettingColorPalette component, keeping UI consistent.
resources/lang/en.json (1)
370-387: Localization keys are well-organized.The new keys follow the existing naming pattern and provide clear descriptions. Grouping under
user_settingkeeps them discoverable.src/client/UserSettingModal.ts (5)
261-279: Theme mode change handler dispatches both events correctly.Dispatching
theme-mode-changedfor the new system anddark-mode-changedfor backward compatibility withDarkModeButtonis the right approach. Usingwindow.dispatchEventfor the global event matches the pattern inDarkModeButton.
707-710: Investigate the need for requestAnimationFrame workaround.The comment says this fixes "initial render issues with columns layout." This suggests a race condition between modal opening and Lit rendering. If this is consistently needed, it's fine, but if the root cause is a styling issue, fixing that would be cleaner.
281-311: Display settings structure is clean.The new
renderDisplaySettingsmethod groups theme, skins, and colors logically. Usingdisabledon the placeholder components clearly signals "coming soon" features.
313-436: Basic settings reorganization with groups looks good.Moving settings into collapsible groups (Interface, Graphics, Controls, Privacy) improves the UI organization. The
columnsproperty on groups enables the two-column layout where appropriate.
86-109:toggleDarkModeis not dead code—it remains in use by other components.The method is still called from
DarkModeButton.tsandSettingsModal.ts, so removing it would break those components. WithinUserSettingModal.tsitself, the template has migrated tohandleThemeModeChange(line 261) for the new theme mode system, but the oldtoggleDarkModemethod serves a different purpose and should be retained for backward compatibility with the dark mode toggle in other files.Likely an incorrect or invalid review comment.
src/client/components/baseComponents/setting/SettingColorPalette.ts (2)
450-522: LGTM - well-structured color palette component.The render method creates a clean, functional color picker with proper event bindings and conditional disabled state. The use of inline styles for dynamic values is appropriate for a Lit component.
One minor note: the disabled badge has
z-index: 1(line 271), which should work but might conflict if future changes add overlays. Consider bumping to a higher value likez-index: 10for safety.
72-274: Clean and themeable CSS implementation.The styles are well-organized with proper use of CSS variables for theming support. The color picker UI elements (gradient overlay, cursor, hue slider) are positioned correctly using absolute positioning and transforms.
src/core/game/UserSettings.ts (3)
106-111: Clean abstraction for checking active theme.
isDarkModeActive()provides a simple boolean API that properly resolves all three theme modes, including system preference. This maintains backward compatibility for components that just need to know the current theme state.
176-188: Correct toggle implementation with event sync.The toggle correctly switches between light and dark modes (bypassing system mode for the button), and dispatches an event for other components to synchronize. The event detail correctly carries the new state.
126-130: Intentional feature disable noted.The focus lock feature is temporarily disabled pending performance fixes. The TODO comment clearly documents this.
src/client/styles/components/setting.css (5)
9-27: Solid responsive layout for settings modal.The fixed 720px width provides consistency while the media query ensures mobile compatibility. The grouped layout using
align-items: stretchallows child components likeSettingGroupto control their own width.
29-50: Proper use of CSS variables and group overrides.The CSS variable updates enable theme switching, and the
setting-group .setting-itemselector correctly allows items inside groups to stretch to full width. The!importantoverrides are necessary to defeat the fixed 360px width from the base.setting-itemrule.
355-397: Well-designed placeholder for upcoming cosmetics.The placeholder styling clearly communicates "coming soon" with appropriate visual hierarchy and subtle yellow accent. The dashed border and low opacity effectively convey the disabled state.
399-442: Clean sticky tabs with modern CSS.The sticky tabs with backdrop blur create a polished UI. The negative margins and width calculation properly extend the tabs to the modal edges. The vertical dividers using
::afterpseudo-elements are a clean solution.Note:
backdrop-filterisn't supported in older Firefox versions, but this is fine as a progressive enhancement—the tabs will still function, just without the blur effect.
33-34: Consistent CSS variable theming system.The CSS variables are well-named, consistently applied, and include sensible fallbacks. This creates a flexible theming system that integrates with the broader theme mode changes in this PR.
Also applies to: 40-40, 150-150, 170-170, 245-245, 251-251, 314-314, 330-330, 404-404, 416-416, 432-432, 440-440
src/client/components/baseComponents/setting/SettingTerritorySkins.ts
Outdated
Show resolved
Hide resolved
- Remove SettingColorPalette.ts (523 lines) and SettingTerritorySkins.ts (442 lines) - Add simple SettingPlaceholder.ts component (~55 lines) - Add placeholder screenshots for cosmetics preview - Reduces PR complexity significantly while maintaining visual preview 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
I've removed the placeholder components for theme and colors, and just put screenshots for placement. This should simplify the review a bunch. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/components/baseComponents/setting/SettingPlaceholder.ts (1)
50-58: Hardcoded badge text should usetranslateText()for i18n.The rest of the settings UI uses
translateText()for user-facing strings. The badge text "Cosmetics Module Required" breaks this pattern. Also, ifimageis empty, the<img src="">may trigger browser console warnings or an unwanted network request.Proposed fix
+import { translateText } from "../../../../client/Utils"; + @customElement("setting-placeholder") export class SettingPlaceholder extends LitElement { - @property({ type: String }) image = ""; + @property({ type: String }) image?: string; @property({ type: String }) alt = "Coming soon";And in the render method:
render() { return html` <div class="placeholder-container"> - <img class="placeholder-image" src="${this.image}" alt="${this.alt}" /> + ${this.image + ? html`<img class="placeholder-image" src="${this.image}" alt="${this.alt}" />` + : null} <div class="placeholder-overlay"> - <div class="placeholder-badge">Cosmetics Module Required</div> + <div class="placeholder-badge">${translateText("user_setting.cosmetics_note")}</div> </div> </div> `; }src/client/UserSettingModal.ts (2)
299-314: Consider translating alt texts for accessibility.The
altattributes ("Territory Skins Preview", "Color Palette Preview") are English strings. For full i18n support, these should usetranslateText(). This is minor since alt text is less visible than UI text.
406-415: Edge case: invalid localStorage value could produce NaN.If
localStorage.getItem("settings.attackRatio")returns a non-numeric string,Number()returnsNaN, causing the slider to receiveNaN. This is unlikely but worth guarding.Defensive parsing
- .value=${Number( - localStorage.getItem("settings.attackRatio") ?? "0.2", - ) * 100} + .value=${(() => { + const raw = localStorage.getItem("settings.attackRatio"); + const parsed = raw !== null ? parseFloat(raw) : 0.2; + return Number.isFinite(parsed) ? parsed * 100 : 20; + })()}Or extract to a helper method for cleaner templates.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
resources/images/placeholders/placeholder-display-color-manager.jpgis excluded by!**/*.jpgresources/images/placeholders/placeholder-display-theme-manager.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
src/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingPlaceholder.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/UserSettingModal.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/UserSettingModal.ts
🧬 Code graph analysis (1)
src/client/components/baseComponents/setting/SettingPlaceholder.ts (1)
src/client/UserSettingModal.ts (1)
customElement(14-721)
🔇 Additional comments (5)
src/client/components/baseComponents/setting/SettingPlaceholder.ts (1)
1-7: LGTM on component structure.Clean, idiomatic Lit component with typed properties and encapsulated styles. The use of composition here (slot-ready, reusable placeholder) fits well with the grouped settings pattern.
src/client/UserSettingModal.ts (4)
4-11: LGTM on new imports.Clean import organization with side-effect imports for custom elements and named imports for types and classes. Follows existing codebase pattern.
18-18: Good use of typed union for settings mode.Extending the union type with
"display"is clean and type-safe.
260-278: LGTM on theme mode handling.Good separation: composed event for parent components, window event for global DarkModeButton sync. The truthy check on
modeis fine since ThemeMode values ("light", "dark", "system") are all truthy strings.
709-716: Acceptable workaround for layout timing.The
requestAnimationFrame+requestUpdatepattern is a common fix for initial render issues when opening modals. The comment explains the intent well.
Screenshots showing: - Settings tab with grouped toggles (Interface, Graphics, Controls, Privacy) - Keybinds tab with categorized key bindings - Display tab with theme mode selector and cosmetics placeholders 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove hardcoded badge overlay from SettingPlaceholder (already in image) - Add translation keys for placeholder alt text - Use translateText() for alt attributes in Display tab - Fix NaN edge case when parsing attackRatio from localStorage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/UserSettingModal.ts (1)
406-418: Consider extracting the attack ratio calculation.The IIFE correctly handles the
NaNedge case, but inline logic in templates can be hard to read. Consider a simple getter or private method for clarity.🔎 Proposed refactor
+ private getAttackRatioPercentage(): number { + const stored = localStorage.getItem("settings.attackRatio"); + const parsed = stored !== null ? Number(stored) : 0.2; + return (Number.isNaN(parsed) ? 0.2 : parsed) * 100; + }Then in the template:
<setting-slider label="${translateText("user_setting.attack_ratio_label")}" description="${translateText("user_setting.attack_ratio_desc")}" min="1" max="100" - .value=${(() => { - const stored = localStorage.getItem("settings.attackRatio"); - const parsed = stored !== null ? Number(stored) : 0.2; - return (Number.isNaN(parsed) ? 0.2 : parsed) * 100; - })()} + .value=${this.getAttackRatioPercentage()} @change=${this.sliderAttackRatio} ></setting-slider>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
resources/images/placeholders/user-settings--display.jpgis excluded by!**/*.jpgresources/images/placeholders/user-settings--keybinds.jpgis excluded by!**/*.jpgresources/images/placeholders/user-settings--settings.jpgis excluded by!**/*.jpg
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingPlaceholder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/components/baseComponents/setting/SettingPlaceholder.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/UserSettingModal.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/UserSettingModal.ts
🧬 Code graph analysis (1)
src/client/UserSettingModal.ts (1)
src/core/game/UserSettings.ts (2)
UserSettings(8-260)ThemeMode(6-6)
🔇 Additional comments (9)
src/client/UserSettingModal.ts (7)
4-18: LGTM!Clean import organization. The
ThemeModetype import and new component side-effect imports follow the established patterns. ThesettingsModetype union correctly includes the new"display"option.
220-257: LGTM!The three-tab layout is well structured. Using a simple union type for
settingsModeand conditional class application keeps things straightforward. The ternary chain for rendering the correct settings section is easy to follow.
260-278: LGTM!The dual event dispatch pattern is clear:
theme-mode-changedbubbles up for modal consumers, whiledark-mode-changedonwindowsyncs theDarkModeButton. The truthy check onmodeis sufficient since all validThemeModevalues ("light","dark","system") are truthy strings.
318-443: LGTM!The grouped settings structure is well organized: Interface, Graphics, Controls, Privacy, and the conditional Easter group. Each
setting-grouphas consistent props (label,groupId,columns). All text usestranslateTextfor localization.
711-718: Acceptable workaround for layout timing.The
requestAnimationFrame+requestUpdatecombo is a known pattern for fixing Lit render timing issues with complex layouts. The comment explains the intent clearly.
481-708: LGTM!The keybind groups (View Options, Build Controls, Attack Controls, Camera Movement) are logically organized. Each
setting-keybindfollows a consistent pattern withaction,label,description,defaultKey, and.valuebinding. All text is localized.
280-316: Component structure is clean and image paths are correct.The use of
translateTextfor alt attributes supports both accessibility and localization. The placeholder images at/images/placeholders/are properly copied to thestatic/directory during the webpack build process via the CopyPlugin, so the file references will work as expected at runtime.resources/lang/en.json (2)
370-389: LGTM!The new localization keys align with the UI components added in
UserSettingModal.ts. Keys follow the existinguser_setting.*naming convention. The descriptions are clear and user-friendly.
391-412: LGTM!Minor wording adjustments and emoji removal for cleaner labels. These changes are consistent with the grouped settings UI style.
on lightmode the text is white on white |
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
on light mode, the player modal has white text on white background
3 participants
Description:
Add Settings UI improvements with a new Display tab and theme mode support.
Features:
Screenshots
Summary of Changes
SettingThemeMode.tsSettingGroup.tsSettingPlaceholder.tsUserSettingModal.tsModal.tsUserSettings.tsDarkModeButton.tsMain.tssetting.cssen.jsonTest Plan
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
nomadic_mondo🤖 Generated with Claude Code