Add lobby info display to the join private lobby modal #2211
Conversation
|
|
WalkthroughAdds many private_lobby localization keys and introduces reusable UI components (GameOptionsDisplay, LobbyCard, CheckboxCard, BotSliderCard, MaxTimerCard). Refactors HostLobbyModal, SinglePlayerModal, JoinPrivateLobbyModal, and PublicLobby to use these components, consolidates option handling (debounced bots), removes RenderUnitTypeOptions, and tightens Transport logging. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant HostLobbyModal
participant GameOptionsDisplay
participant Transport
rect rgba(220,240,255,0.6)
HostLobbyModal->>GameOptionsDisplay: render(gameConfig, bots, editable=true, callbacks)
end
User->>GameOptionsDisplay: toggle option / unit
GameOptionsDisplay-->>HostLobbyModal: onOptionChange(key, value)
HostLobbyModal->>HostLobbyModal: update local state
HostLobbyModal->>Transport: PUT game config
User->>GameOptionsDisplay: adjust bots slider
GameOptionsDisplay-->>HostLobbyModal: onBotsChange(value)
HostLobbyModal->>HostLobbyModal: update UI immediately
note right of HostLobbyModal: debounce 300ms before network update
HostLobbyModal->>Transport: PUT game config (bots)
sequenceDiagram
actor User
participant PublicLobby
participant LobbyCard
PublicLobby->>LobbyCard: render(gameConfig, mapImageSrc, flags, counts, timer)
note right of LobbyCard: load/refresh map image when gameMap changes
User->>LobbyCard: click
LobbyCard-->>PublicLobby: dispatch "card-click"
PublicLobby->>PublicLobby: handle select / debounce
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Files/areas to pay extra attention to:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
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: 2
🧹 Nitpick comments (18)
src/client/Transport.ts (1)
576-579: Apply the fallback pattern consistently across similar log statements.The nullish coalescing operator with
"socket is null"improves log clarity. However, similar log statements at line 562 (onSendWinnerEvent) and line 635 (sendIntent) usethis.socket?.readyStatewithout the fallback. Consider applying the same pattern for consistency.Apply this diff to line 562:
console.log( "WebSocket is not open. Current state:", - this.socket?.readyState, + this.socket?.readyState ?? "socket is null", );And this diff to line 635:
console.log( "WebSocket is not open. Current state:", - this.socket?.readyState, + this.socket?.readyState ?? "socket is null", );resources/lang/en.json (1)
213-240: Drop punctuation in labels; consider reusing existing keys
- "difficulty": "Difficulty: " includes a colon and trailing space. Keep labels punctuation-free; add ":" in UI if needed.
- Optional: reuse existing labels (e.g., map.map, difficulty.difficulty) to reduce translation duplication.
Apply minimal fix:
- "difficulty": "Difficulty: ", + "difficulty": "Difficulty",src/client/JoinPrivateLobbyModal.ts (3)
25-26: Timer type: avoid NodeJS.Timeout in browser codeUse ReturnType to be safe across DOM/Node typings.
- private playersInterval: NodeJS.Timeout | null = null; + private playersInterval: ReturnType<typeof setInterval> | null = null;
32-35: Clear polling on disconnect to avoid leaksIf the element is removed without calling close(), the interval keeps running. Clear it in disconnectedCallback.
disconnectedCallback() { - window.removeEventListener("keydown", this.handleKeyDown); - super.disconnectedCallback(); + window.removeEventListener("keydown", this.handleKeyDown); + if (this.playersInterval) { + clearInterval(this.playersInterval); + this.playersInterval = null; + } + super.disconnectedCallback(); }
198-206: Use the new private_lobby title key for read‑only settingsYou added private_lobby.game_settings; use it here instead of host_modal.options_title for consistency.
- <div class="option-title"> - ${translateText("host_modal.options_title")} - </div> + <div class="option-title"> + ${translateText("private_lobby.game_settings")} + </div>src/client/SinglePlayerModal.ts (3)
222-229: Avoid binding in templates; use arrow properties.bind(this) allocates a new function each render. Convert handlers to arrow properties and pass directly.
- <game-options-display - .gameConfig=${this.getCurrentGameConfig()} - .editable=${true} - .bots=${this.bots} - .onBotsChange=${this.handleBotsChange.bind(this)} - .onOptionChange=${this.handleOptionChange.bind(this)} - .onUnitToggle=${this.toggleUnit.bind(this)} - ></game-options-display> + <game-options-display + .gameConfig=${this.getCurrentGameConfig()} + .editable=${true} + .bots=${this.bots} + .onBotsChange=${this.handleBotsChange} + .onOptionChange=${this.handleOptionChange} + .onUnitToggle=${this.toggleUnit} + ></game-options-display>And update handlers:
- private handleBotsChange(value: number) { + private handleBotsChange = (value: number) => { if (isNaN(value) || value < 0 || value > 400) return; this.bots = value; - } + }- private toggleUnit(unit: UnitType, checked: boolean): void { + private toggleUnit = (unit: UnitType, checked: boolean): void => { this.disabledUnits = checked ? [...this.disabledUnits, unit] : this.disabledUnits.filter((u) => u !== unit); - } + }- private handleOptionChange(key: string, value: boolean) { + private handleOptionChange = ( + key: + | "disableNPCs" + | "instantBuild" + | "infiniteGold" + | "infiniteTroops" + | "compactMap", + value: boolean, + ) => { switch (key) { // cases... } - } + }
317-337: Type the returned game configMake return type explicit and import GameConfig.
-import { TeamCountConfig } from "../core/Schemas"; +import { TeamCountConfig, GameConfig } from "../core/Schemas";- private getCurrentGameConfig() { + private getCurrentGameConfig(): GameConfig { return { // ... }; }
276-295: Remove unused per‑option handlershandleInstantBuildChange, handleInfiniteGoldChange, handleInfiniteTroopsChange, handleCompactMapChange, handleDisableNPCsChange are no longer used with the unified handler.
src/client/PublicLobby.ts (3)
20-21: Strongly type currentMap by enumUse GameMapType rather than string for safer comparisons.
-import { GameMapType } from "../core/game/Game"; +import { GameMapType } from "../core/game/Game"; ... - private lobbyIDToCurrentMap = new Map<GameID, string>(); + private lobbyIDToCurrentMap = new Map<GameID, GameMapType>();
54-62: Avoid string casts for gameMapCarry GameMapType through to image loader.
- if (l.gameConfig) { + if (l.gameConfig) { const currentMap = this.lobbyIDToCurrentMap.get(l.gameID); - if (currentMap !== l.gameConfig.gameMap) { + if (currentMap !== l.gameConfig.gameMap) { // Map has changed or not loaded yet, reload the image - this.loadMapImage(l.gameID, l.gameConfig.gameMap); + this.loadMapImage(l.gameID, l.gameConfig.gameMap); this.lobbyIDToCurrentMap.set(l.gameID, l.gameConfig.gameMap); } }And update the loader signature:
- private async loadMapImage(gameID: GameID, gameMap: string) { + private async loadMapImage(gameID: GameID, gameMap: GameMapType) { try { - // Convert string to GameMapType enum value - const mapType = gameMap as GameMapType; - const data = terrainMapFileLoader.getMapData(mapType); + const data = terrainMapFileLoader.getMapData(gameMap); this.mapImages.set(gameID, await data.webpPath()); this.requestUpdate(); } catch (error) { console.error("Failed to load map image:", error); } }
106-111: Return an empty template when no configSmall polish for clarity.
- if (!lobby?.gameConfig) { - return; - } + if (!lobby?.gameConfig) return html``;src/client/HostLobbyModal.ts (5)
58-61: Timer types: use ReturnType helpersAvoid NodeJS typings in browser code and align timeout type.
- private playersInterval: NodeJS.Timeout | null = null; + private playersInterval: ReturnType<typeof setInterval> | null = null; // Add a new timer for debouncing bot changes - private botsUpdateTimer: number | null = null; + private botsUpdateTimer: ReturnType<typeof setTimeout> | null = null;
315-323: Avoid binding in templates; use arrow property handlersSame as in SinglePlayerModal: convert handlers to arrow properties and drop .bind(this) in the template to reduce allocations.
501-519: Simplify GameType assignmentThe ternary returns the same value; assign directly.
- gameType: - this.gameMode === GameMode.FFA ? GameType.Private : GameType.Private, + gameType: GameType.Private,
630-640: Persist lobby ID visibility toggleYou read lobbyIdVisible from UserSettings on open; also persist when toggled, so it sticks next time.
Example:
private toggleLobbyIdVisibility = () => { this.lobbyIdVisible = !this.lobbyIdVisible; this.userSettings.set("settings.lobbyIdVisibility", this.lobbyIdVisible); };Then in the template: @click=${this.toggleLobbyIdVisibility}
655-658: Remove noisy log in pollingThis runs every second; drop or guard behind DEBUG.
- console.log(`got game info response: ${JSON.stringify(data)}`); + // optionally log in debug modesrc/client/components/GameOptionsDisplay.ts (2)
86-97: Remove duplicate handler logic.Both
@inputand@changehandlers contain identical code that parses the value and callsonBotsChange. The@inputevent fires continuously while dragging the slider, while@changefires only on release. Having both is redundant.Keep only the
@inputhandler for responsive live updates:@input=${(e: Event) => { const value = parseInt((e.target as HTMLInputElement).value); if (this.onBotsChange) { this.onBotsChange(value); } }} - @change=${(e: Event) => { - const value = parseInt((e.target as HTMLInputElement).value); - if (this.onBotsChange) { - this.onBotsChange(value); - } - }} .value="${String(botCount)}"
342-346: Inverted checkbox semantics may confuse users.The checkbox is checked when a unit is disabled (line 342:
.checked=${disabledUnits.includes(type)}). Most UIs use checkboxes to represent "enabled" state, not "disabled" state. Users typically expect checked = enabled, unchecked = disabled.While this is consistent with the
disabledUnitsdata model and theonUnitToggle(unit, disabled)callback signature, consider either:
- Inverting the logic so checked = enabled for better UX
- Adding clear labeling like "Disable [UnitName]" to make the semantics explicit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
resources/lang/en.json(1 hunks)src/client/HostLobbyModal.ts(5 hunks)src/client/JoinPrivateLobbyModal.ts(7 hunks)src/client/PublicLobby.ts(4 hunks)src/client/SinglePlayerModal.ts(4 hunks)src/client/Transport.ts(1 hunks)src/client/components/GameOptionsDisplay.ts(1 hunks)src/client/components/LobbyCard.ts(1 hunks)src/client/utilities/RenderUnitTypeOptions.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/client/utilities/RenderUnitTypeOptions.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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/HostLobbyModal.tssrc/client/PublicLobby.tssrc/client/JoinPrivateLobbyModal.ts
🧬 Code graph analysis (5)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
GameConfig(81-81)
src/client/PublicLobby.ts (1)
src/core/Schemas.ts (1)
GameID(24-24)
src/client/JoinPrivateLobbyModal.ts (2)
src/core/Schemas.ts (2)
GameConfig(81-81)GameInfo(122-128)src/server/GameServer.ts (1)
gameInfo(588-600)
src/client/components/LobbyCard.ts (4)
src/client/components/GameOptionsDisplay.ts (1)
customElement(11-374)src/core/Schemas.ts (1)
GameConfig(81-81)src/client/TerrainMapFileLoader.ts (1)
terrainMapFileLoader(4-4)src/client/Utils.ts (1)
renderDuration(5-13)
src/client/components/GameOptionsDisplay.ts (1)
src/core/Schemas.ts (1)
GameConfig(81-81)
🔇 Additional comments (2)
src/client/HostLobbyModal.ts (1)
442-460: Debounce implementation looks goodBots update debouncing (300ms) balances UX and network load. Nice.
src/client/components/LobbyCard.ts (1)
163-164: Invalid unreachable code assertion.teamCountcan be a string literal ("Duos", "Trios", "Quads") viaGameConfig.playerTeams, so thetypeof teamCount === "string"branch is reachable and should remain.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/components/CheckboxCard.ts(1 hunks)src/client/components/GameOptionsDisplay.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/client/components/CheckboxCard.ts (1)
src/client/components/GameOptionsDisplay.ts (1)
customElement(12-297)
src/client/components/GameOptionsDisplay.ts (4)
src/client/components/CheckboxCard.ts (1)
customElement(9-172)src/client/HostLobbyModal.ts (1)
customElement(32-671)src/client/JoinPrivateLobbyModal.ts (1)
customElement(13-371)src/client/SinglePlayerModal.ts (1)
customElement(30-442)
🔇 Additional comments (1)
src/client/components/GameOptionsDisplay.ts (1)
61-296: Nice reuse of the option renderingI like how you split the sliders, checkboxes, and unit toggles into helpers and wired them through the new card component; reads clean and keeps the UI consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/components/CheckboxCard.ts (1)
127-153: Missing fallback for emptyinputId.When
inputIdis not provided (defaults to""), both the label'sforattribute and the input'sidbecome empty strings. This breaks label-input association. If multipleCheckboxCardinstances exist with emptyinputId, they'll all haveid="", causing DOM conflicts.From the past review comment, the intent was: "Falls back to the host element id when not provided." But that fallback logic is missing.
Apply this diff to add the fallback:
private renderEditable(): TemplateResult { + const inputId = this.inputId || this.id || ""; const style = this.width ? `width: ${this.width};` : ""; return html` <label - for="${this.inputId}" + for="${inputId}" class="option-card ${this.checked ? "selected" : ""}" style="${style}" > <div class="checkbox-icon"></div> <input type="checkbox" - id="${this.inputId}" + id="${inputId}" @change=${(e: Event) => { const checked = (e.target as HTMLInputElement).checked; if (this.onChange) { this.onChange(checked); } }} .checked=${this.checked} /> <div class="option-card-title" style="${this.titleStyle ?? ""}"> ${this.label} </div> </label> `; }
🧹 Nitpick comments (2)
src/client/components/CheckboxCard.ts (2)
158-171: Read-only render omits the checkbox icon.The editable version (line 136) renders
<div class="checkbox-icon"></div>to show a checkmark when selected. The read-only version doesn't include this, so users see only text in a card—no visual indicator of the checked state beyond the border color.If the icon is intentionally omitted for a cleaner read-only display, that's fine. Otherwise, consider adding it for consistency.
Apply this diff if you want the icon in read-only mode:
private renderReadOnly(): TemplateResult { const style = this.width ? `width: ${this.width};` : ""; return html` <div class="option-card ${this.checked ? "selected" : ""}" style="pointer-events: none; ${style}" > + <div class="checkbox-icon"></div> <div class="option-card-title" style="${this.titleStyle ?? ""}"> ${this.label} </div> </div> `; }
89-91: Redundant hover transform override.Line 67 sets
transform: translateY(-0.125rem)on.option-card:hover. Lines 89-91 then override it totransform: noneforlabel.option-card:hover. This is confusing: why set a transform only to immediately cancel it?If the intent is to prevent hover lift on labels, consider removing the override and using a more specific selector instead.
Apply this diff to simplify:
- label.option-card:hover { - transform: none; - }Then adjust the hover rule to exclude labels if needed:
- .option-card:hover { + .option-card:hover:not(label) { transform: translateY(-0.125rem); border-color: rgba(255, 255, 255, 0.3); background: rgba(40, 40, 40, 0.95); }Or simply accept that labels won't lift on hover (since they're interactive via the hidden checkbox).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/components/CheckboxCard.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/components/CheckboxCard.ts (1)
src/client/components/GameOptionsDisplay.ts (1)
customElement(12-297)
|
Seems like there's a bug, I can't select options: Screen.Recording.2025-10-24.at.12.31.38.PM.mov |
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
github-actions
bot
removed
the
Stale
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/client/components/MaxTimerCard.ts (1)
41-66: Consider extracting shared card styles.The
.option-cardbase styles (lines 41-66) are duplicated inBotSliderCard.ts. Moving common card styling to a shared stylesheet or base class would reduce duplication and simplify future updates.src/client/components/BotSliderCard.ts (2)
11-60: Consider conditional styling for editable/non-editable states.The
.option-cardhover effects andcursor: pointer(lines 28, 32-36) apply to all cards, but non-editable cards disable interaction with an inlinepointer-events: nonestyle (line 88). This creates a minor inconsistency where the CSS suggests interactivity that isn't present.Consider adding a
.option-card.non-editableclass or using:host([editable])selectors to conditionally apply interactive styling only when the component is editable.Apply this pattern to make styling reflect behavior:
.option-card { /* ... base styles ... */ - cursor: pointer; transition: all 0.2s ease-in-out; } + .option-card.editable { + cursor: pointer; + } + - .option-card:hover { + .option-card.editable:hover { transform: translateY(-0.125rem); border-color: rgba(255, 255, 255, 0.3); background: rgba(40, 40, 40, 0.95); }Then update the render methods to use the class:
// Editable mode: <label for="bots-count" class="option-card editable"> // Non-editable mode: <div class="option-card ${this.bots > 0 ? "selected" : ""}" >
78-80: Extract duplicate bot display logic.The bot count display logic appears twice (lines 78-80 and 100-102) with identical ternary expressions. Extract this to a helper method to follow the DRY principle.
+ private renderBotCount() { + return this.bots === 0 + ? translateText("host_modal.bots_disabled") + : this.bots; + } + render() { if (this.editable) { return html` <label for="bots-count" class="option-card"> <!-- ... range input ... --> <div class="option-card-title"> <span>${translateText("host_modal.bots")} </span> - ${this.bots === 0 - ? translateText("host_modal.bots_disabled") - : this.bots} + ${this.renderBotCount()} </div> </label> `; } else { return html` <div class="option-card ${this.bots > 0 ? "selected" : ""}"> <!-- ... disabled range input ... --> <div class="option-card-title"> <span>${translateText("host_modal.bots")} </span> - ${this.bots === 0 - ? translateText("host_modal.bots_disabled") - : this.bots} + ${this.renderBotCount()} </div> </div> `; } }Also applies to: 100-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/components/BotSliderCard.ts(1 hunks)src/client/components/MaxTimerCard.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
🧬 Code graph analysis (1)
src/client/components/MaxTimerCard.ts (1)
src/client/components/BotSliderCard.ts (1)
customElement(5-122)
🔇 Additional comments (4)
src/client/components/MaxTimerCard.ts (2)
172-190: LGTM! Input validation is thorough and correct.The dual approach (keydown prevention + input sanitization) effectively blocks invalid characters (
e,+,-) and out-of-range values. The 0-120 constraint matches the input attributes.
117-170: Well-structured component with clear API.The editable/non-editable modes are cleanly separated, toggle semantics are intuitive (passing the desired enabled state), and event handling properly manages propagation. The component integrates well with the shared game-options UI pattern introduced in this PR.
src/client/components/BotSliderCard.ts (2)
1-9: LGTM!Clean component setup with well-typed properties and appropriate Lit decorators.
72-73: Dual event handlers are properly managed—no issue found.The concern about dual
@input/@changefiring causing performance problems does not apply here:
HostLobbyModal: Uses a debouncing pattern (300ms timeout). When
@inputfires repeatedly during drag, it resets the timer. When@changefires on release, the timer is already set. Result:putGameConfig()is called only once per user interaction, not twice.SinglePlayerModal: Calls only update local state (
this.bots); no expensive operations occur.The code is well-designed and handles dual events safely. No changes needed.
| <input | ||
| type="range" | ||
| id="bots-count" | ||
| min="0" | ||
| max="400" | ||
| step="1" | ||
| @input=${this.handleInput} | ||
| @change=${this.handleChange} | ||
| .value="${String(this.bots)}" | ||
| /> |
There was a problem hiding this comment.
Add accessibility attributes to range inputs.
Both range inputs lack aria-label or aria-describedby attributes, which means screen readers won't convey what the slider controls. This impacts users relying on assistive technology.
Apply this diff to add accessible labels:
<label for="bots-count" class="option-card">
<input
type="range"
id="bots-count"
+ aria-label="${translateText("host_modal.bots")}"
+ aria-valuetext="${this.bots === 0 ? translateText("host_modal.bots_disabled") : String(this.bots)}"
min="0"
max="400"
step="1"
@input=${this.handleInput}
@change=${this.handleChange}
.value="${String(this.bots)}"
/>
<!-- ... -->
</label>
<!-- Non-editable mode -->
<div class="option-card ${this.bots > 0 ? "selected" : ""}">
<input
type="range"
+ aria-label="${translateText("host_modal.bots")}"
+ aria-valuetext="${this.bots === 0 ? translateText("host_modal.bots_disabled") : String(this.bots)}"
min="0"
max="400"
step="1"
.value="${String(this.bots)}"
disabled
/>
<!-- ... -->
</div>Also applies to: 90-97
🤖 Prompt for AI Agents
In src/client/components/BotSliderCard.ts around lines 66-75 and 90-97 the range
inputs are missing accessible labeling, so screen readers won't identify what
each slider controls; add either aria-label or aria-describedby attributes to
each <input type="range"> (e.g., aria-label="Number of bots" or aria-describedby
pointing to a visible label/ID) and ensure the label text or description ID
exists and is unique so assistive tech can convey the control purpose and
current value.
| private handleInput(e: Event) { | ||
| const value = parseInt((e.target as HTMLInputElement).value); | ||
| if (this.onBotsChange) { | ||
| this.onBotsChange(value); | ||
| } | ||
| } | ||
|
|
||
| private handleChange(e: Event) { | ||
| const value = parseInt((e.target as HTMLInputElement).value); | ||
| if (this.onBotsChange) { | ||
| this.onBotsChange(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate duplicate event handlers.
Both handleInput and handleChange have identical implementations. This violates the DRY principle and creates unnecessary maintenance burden.
Apply this diff to consolidate:
- private handleInput(e: Event) {
- const value = parseInt((e.target as HTMLInputElement).value);
+ private handleValueChange(e: Event) {
+ const value = parseInt((e.target as HTMLInputElement).value, 10);
+ if (isNaN(value)) return;
if (this.onBotsChange) {
this.onBotsChange(value);
}
}
- private handleChange(e: Event) {
- const value = parseInt((e.target as HTMLInputElement).value);
- if (this.onBotsChange) {
- this.onBotsChange(value);
- }
- }Then update the event bindings:
<input
type="range"
id="bots-count"
min="0"
max="400"
step="1"
- @input=${this.handleInput}
- @change=${this.handleChange}
+ @input=${this.handleValueChange}
+ @change=${this.handleValueChange}
.value="${String(this.bots)}"
/>Note: Also added radix parameter 10 to parseInt and a NaN check for defensive coding.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/client/components/BotSliderCard.ts around lines 109 to 121, the
handleInput and handleChange functions are duplicates; replace them with a
single handler (e.g., handleBotsChange) that reads the value via
parseInt((e.target as HTMLInputElement).value, 10), checks for Number.isNaN and
returns early if invalid, and then calls this.onBotsChange(value) when present;
update all event bindings to use the new single handler and remove the redundant
method.
| .option-card input[type="checkbox"] { | ||
| opacity: 0; | ||
| position: absolute; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
Remove unused checkbox input styles.
The template doesn't include an input[type="checkbox"] element—only a div with class checkbox-icon. These CSS rules are dead code.
Apply this diff to remove the unused CSS:
- .option-card input[type="checkbox"] {
- opacity: 0;
- position: absolute;
- pointer-events: none;
- }
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .option-card input[type="checkbox"] { | |
| opacity: 0; | |
| position: absolute; | |
| pointer-events: none; | |
| } |
🤖 Prompt for AI Agents
In src/client/components/MaxTimerCard.ts around lines 75 to 79, the CSS block
targeting .option-card input[type="checkbox"] is unused because the template
uses a div.checkbox-icon instead of an input; remove that entire selector and
its rules (opacity, position, pointer-events) to eliminate dead CSS and keep
styles focused on the actual DOM elements.
6 participants
Description:
Adds UI to show the private lobby setup when joining a private lobby. Reuses the public lobby card to show the map, game mode and difficulty, reuses the config section from the host modal for showing the settings and enabled units - with click affordances removed. Reused these components to make future changes easier as you dont need to remember to update 2 places
Video demo: https://youtu.be/1YYm2Il1Ep8
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
ninjachicken737