UI: split/classic control panel variants and linked attack/unit layout#3376
Open
hkio120 wants to merge 1 commit intoopenfrontio:mainfrom
Open
UI: split/classic control panel variants and linked attack/unit layout#3376hkio120 wants to merge 1 commit intoopenfrontio:mainfrom
hkio120 wants to merge 1 commit intoopenfrontio:mainfrom
Conversation
Contributor
WalkthroughThe pull request adds alternate in-game layout options (Classic vs Split) for the HUD with localStorage persistence, reorganizes the attack display into incoming/outgoing columns, enhances the control panel with gold-per-minute income tracking and troop regeneration color optimization, and implements a centralized tooltip system for unit displays. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Button
participant localStorage
participant CSS
participant DOM
User->>Button: Click layout-switch or style-button
Button->>localStorage: Read current layout/style
localStorage-->>Button: Return stored value
Button->>localStorage: Write new layout/style preference
Button->>CSS: Apply layout/style class to document
CSS->>DOM: Update element positioning and appearance
DOM-->>User: Render new layout
Button->>Button: Update button label text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/UnitDisplay.ts (1)
266-318:⚠️ Potential issue | 🟠 MajorUse a keyboard-focusable control here.
This is still a clickable
divwith mouse-only handlers, so keyboard and assistive-tech users cannot focus it, trigger it, or get the same highlight/tooltip behavior. Please switch it to a real<button type="button">or addtabindex,role="button", and matchingfocus/blur/keydownhandlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitDisplay.ts` around lines 266 - 318, The clickable div used for unit tiles is not keyboard-focusable; update the element in UnitDisplay (the block that uses this.canBuild(unitType), selected, this.uiState.ghostStructure, this.eventBus and emits GhostStructureChangedEvent/ToggleStructureEvent) to be an actual <button type="button> or, if you must keep a div, add tabindex="0", role="button", and wire up focus/blur handlers plus a keydown handler that triggers the same click behavior on Enter/Space and the same mouseenter/mouseleave behavior for keyboard focus; ensure the handlers call the same logic that currently runs in the `@click`, `@mouseenter` and `@mouseleave` callbacks (including canBuild checks, updating uiState.ghostStructure and emitting GhostStructureChangedEvent/ToggleStructureEvent) and call this.requestUpdate() so keyboard users get identical highlighting/tooltip behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 536-543: The range inputs for attack ratio in ControlPanel (the
inputs bound to this.attackRatio and using handleRatioSliderInput) are unlabeled
for screen readers; add an accessible label to each of the three inputs (the
ones using .value=${String(Math.round(this.attackRatio * 100))} and attached to
handleRatioSliderInput) by adding either an aria-label="Attack ratio" (or
similarly descriptive text per context) or an aria-labelledby that points to a
visible label element ID so screen readers can identify the control; ensure you
apply the same change to the other two matching inputs in the file.
- Around line 166-179: handleBarTouch currently measures the popup rect and only
runs on touchstart, causing jumpy behavior; update the code so the touch handler
is attached to the actual bar element (query/select the ".attack-drag-bar"
element) and bind it for both "touchstart" and "touchmove", then inside
handleBarTouch compute the rect from that bar element (not the popup) and use
touch.clientY-relative-to-bar to compute ratio, clamp and set attackRatio,
persist and call onAttackRatioChange, requestUpdate and preventDefault; apply
the same change to the other similar handlers referenced (the other touchstart
handlers) so all touch interactions track finger movement.
- Around line 292-334: The current dedup logic uses only amount strings
(tradeCounts keyed by g.toString()) so train BonusEvent entries with the same
gold as a trade display get incorrectly removed; change the matching to include
the event source/type or only deduplicate captured-ship displays: when building
tradeCounts from displayEvents include the display message (or an identifier
like "captured_ship" vs "trade") in the key (e.g., `${event.message}:${amount}`)
and when iterating bonusEvents use the BonusEventUpdate's source/type field (or
only consider deduping if the display message is
"events_display.received_gold_from_captured_ship") to form the same composite
key before decrementing tradeCounts; update places referencing
tradeDisplayGolds, bonusGolds, tradeCounts, trainGold, and sumBigInts
accordingly so only captured-ship bonus entries are removed rather than any
same-valued train income.
In `@src/client/graphics/layers/UnitDisplay.ts`:
- Around line 142-160: The tooltip is currently positioned with hard-coded
viewport coordinates (CSS classes fixed top-[22rem] left-4) so it drifts; in
UnitDisplay.ts update the tooltip rendering to anchor to the hovered element or
panel container instead: remove the fixed/top/left classes and compute position
using the hovered item's DOM bounds (use the element that corresponds to
this._hoveredUnit or the panel container), set inline style top/left (or use
absolute positioning within a relatively positioned container) when rendering
the tooltip that shows this._hoveredStructureKey / this._hoveredDisplayHotkey /
renderNumber(this.cost(this._hoveredUnit)) with goldCoinIcon; ensure fallback
positioning if bounds are unavailable and recalc on layout changes or window
resize.
---
Outside diff comments:
In `@src/client/graphics/layers/UnitDisplay.ts`:
- Around line 266-318: The clickable div used for unit tiles is not
keyboard-focusable; update the element in UnitDisplay (the block that uses
this.canBuild(unitType), selected, this.uiState.ghostStructure, this.eventBus
and emits GhostStructureChangedEvent/ToggleStructureEvent) to be an actual
<button type="button> or, if you must keep a div, add tabindex="0",
role="button", and wire up focus/blur handlers plus a keydown handler that
triggers the same click behavior on Enter/Space and the same
mouseenter/mouseleave behavior for keyboard focus; ensure the handlers call the
same logic that currently runs in the `@click`, `@mouseenter` and `@mouseleave`
callbacks (including canBuild checks, updating uiState.ghostStructure and
emitting GhostStructureChangedEvent/ToggleStructureEvent) and call
this.requestUpdate() so keyboard users get identical highlighting/tooltip
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4535cdca-1c10-4734-b666-3b48b1eb8994
📒 Files selected for processing (4)
index.htmlsrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/UnitDisplay.ts
Comment on lines
+166
to
+179
| private handleBarTouch(e: TouchEvent) { | ||
| // original touch-drag handler (keep existing behavior) | ||
| const touch = e.touches[0]; | ||
| const target = e.currentTarget as HTMLElement; | ||
| const rect = target.getBoundingClientRect(); | ||
| const y = touch.clientY - rect.top; | ||
| const ratio = 1 - y / rect.height; | ||
| const clamped = Math.min(1, Math.max(0.01, ratio)); | ||
| this.attackRatio = clamped; | ||
| localStorage.setItem("settings.attackRatio", String(this.attackRatio)); | ||
| this.onAttackRatioChange(this.attackRatio); | ||
| this.requestUpdate(); | ||
| e.preventDefault(); | ||
| } |
Contributor
There was a problem hiding this comment.
Make the mobile ratio popup track finger movement.
Right now handleBarTouch only runs on touchstart, and it reads the rect of the whole popup instead of the .attack-drag-bar. That turns the control into a jump-to-value tap and skews the math near the label and padding. Bind the handler to the bar itself on both touchstart and touchmove.
Also applies to: 628-643, 736-750
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` around lines 166 - 179,
handleBarTouch currently measures the popup rect and only runs on touchstart,
causing jumpy behavior; update the code so the touch handler is attached to the
actual bar element (query/select the ".attack-drag-bar" element) and bind it for
both "touchstart" and "touchmove", then inside handleBarTouch compute the rect
from that bar element (not the popup) and use touch.clientY-relative-to-bar to
compute ratio, clamp and set attackRatio, persist and call onAttackRatioChange,
requestUpdate and preventDefault; apply the same change to the other similar
handlers referenced (the other touchstart handlers) so all touch interactions
track finger movement.
Comment on lines
+292
to
+334
| const tradeDisplayGolds: bigint[] = []; | ||
| const displayEvents = (updates[GameUpdateType.DisplayEvent] ?? | ||
| []) as DisplayMessageUpdate[]; | ||
| for (const event of displayEvents) { | ||
| if (event.playerID !== mySmallId || event.goldAmount === undefined) | ||
| continue; | ||
| if ( | ||
| event.message === "events_display.received_gold_from_trade" || | ||
| event.message === "events_display.received_gold_from_captured_ship" | ||
| ) { | ||
| tradeDisplayGolds.push(event.goldAmount); | ||
| } | ||
| } | ||
|
|
||
| const bonusGolds: bigint[] = []; | ||
| const bonusEvents = (updates[GameUpdateType.BonusEvent] ?? | ||
| []) as BonusEventUpdate[]; | ||
| for (const event of bonusEvents) { | ||
| if (event.player !== myId || event.gold <= 0) continue; | ||
| bonusGolds.push(BigInt(Math.max(0, Math.floor(event.gold)))); | ||
| } | ||
|
|
||
| // BonusEvent includes train and some ship gains (destination/captured). | ||
| // Remove bonus entries that match ship display messages, keep remaining as train gain. | ||
| const tradeCounts = new Map<string, number>(); | ||
| for (const g of tradeDisplayGolds) { | ||
| const key = g.toString(); | ||
| tradeCounts.set(key, (tradeCounts.get(key) ?? 0) + 1); | ||
| } | ||
|
|
||
| let trainGold = 0n; | ||
| for (const bg of bonusGolds) { | ||
| const key = bg.toString(); | ||
| const remaining = tradeCounts.get(key) ?? 0; | ||
| if (remaining > 0) { | ||
| tradeCounts.set(key, remaining - 1); | ||
| } else { | ||
| trainGold += bg; | ||
| } | ||
| } | ||
|
|
||
| const shipGold = this.sumBigInts(tradeDisplayGolds); | ||
| const combined = shipGold + trainGold; |
Contributor
There was a problem hiding this comment.
Do not match train income against trade income by amount alone.
A trade display event and a train bonus can have the same gold value in the same tick. With the current Map<string, number> keyed only by amount, that train entry gets discarded and the +.../m badge undercounts. Only de-duplicate captured-ship events against BonusEvent, or include the source in the match key.
Suggested fix
- const tradeDisplayGolds: bigint[] = [];
+ const tradeGolds: bigint[] = [];
+ const capturedShipDisplayGolds: bigint[] = [];
- if (
- event.message === "events_display.received_gold_from_trade" ||
- event.message === "events_display.received_gold_from_captured_ship"
- ) {
- tradeDisplayGolds.push(event.goldAmount);
- }
+ if (event.message === "events_display.received_gold_from_trade") {
+ tradeGolds.push(event.goldAmount);
+ }
+ if (event.message === "events_display.received_gold_from_captured_ship") {
+ capturedShipDisplayGolds.push(event.goldAmount);
+ }
- const tradeCounts = new Map<string, number>();
- for (const g of tradeDisplayGolds) {
+ const capturedShipCounts = new Map<string, number>();
+ for (const g of capturedShipDisplayGolds) {
const key = g.toString();
- tradeCounts.set(key, (tradeCounts.get(key) ?? 0) + 1);
+ capturedShipCounts.set(key, (capturedShipCounts.get(key) ?? 0) + 1);
}
- const remaining = tradeCounts.get(key) ?? 0;
+ const remaining = capturedShipCounts.get(key) ?? 0;
if (remaining > 0) {
- tradeCounts.set(key, remaining - 1);
+ capturedShipCounts.set(key, remaining - 1);
} else {
trainGold += bg;
}
- const shipGold = this.sumBigInts(tradeDisplayGolds);
- const combined = shipGold + trainGold;
+ const combined =
+ this.sumBigInts(tradeGolds) +
+ this.sumBigInts(capturedShipDisplayGolds) +
+ trainGold;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` around lines 292 - 334, The
current dedup logic uses only amount strings (tradeCounts keyed by g.toString())
so train BonusEvent entries with the same gold as a trade display get
incorrectly removed; change the matching to include the event source/type or
only deduplicate captured-ship displays: when building tradeCounts from
displayEvents include the display message (or an identifier like "captured_ship"
vs "trade") in the key (e.g., `${event.message}:${amount}`) and when iterating
bonusEvents use the BonusEventUpdate's source/type field (or only consider
deduping if the display message is
"events_display.received_gold_from_captured_ship") to form the same composite
key before decrementing tradeCounts; update places referencing
tradeDisplayGolds, bonusGolds, tradeCounts, trainGold, and sumBigInts
accordingly so only captured-ship bonus entries are removed rather than any
same-valued train income.
Comment on lines
+536
to
+543
| <input | ||
| type="range" | ||
| min="1" | ||
| max="100" | ||
| .value=${String(Math.round(this.attackRatio * 100))} | ||
| @input=${(e: Event) => this.handleRatioSliderInput(e)} | ||
| class="mt-1 w-full min-w-0 h-1.5 accent-blue-400 cursor-pointer" | ||
| /> |
Contributor
There was a problem hiding this comment.
Label the new attack-ratio sliders for assistive tech.
These input[type="range"] controls only have nearby text, so screen readers will hit an unlabeled slider in each layout branch. Please add aria-label or aria-labelledby to all three inputs.
Also applies to: 579-586, 761-768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` around lines 536 - 543, The range
inputs for attack ratio in ControlPanel (the inputs bound to this.attackRatio
and using handleRatioSliderInput) are unlabeled for screen readers; add an
accessible label to each of the three inputs (the ones using
.value=${String(Math.round(this.attackRatio * 100))} and attached to
handleRatioSliderInput) by adding either an aria-label="Attack ratio" (or
similarly descriptive text per context) or an aria-labelledby that points to a
visible label element ID so screen readers can identify the control; ensure you
apply the same change to the other two matching inputs in the file.
Comment on lines
+142
to
+160
| <div | ||
| class="fixed top-[22rem] left-4 text-gray-200 text-center w-max text-xs bg-gray-800/95 backdrop-blur-xs rounded-sm p-1 z-[5000] shadow-lg pointer-events-none border border-white/10" | ||
| > | ||
| <div class="font-bold text-sm mb-1"> | ||
| ${translateText("unit_type." + this._hoveredStructureKey)}${` [${this | ||
| ._hoveredDisplayHotkey}]`} | ||
| </div> | ||
| <div class="p-2"> | ||
| ${translateText( | ||
| "build_menu.desc." + this._hoveredStructureKey, | ||
| )} | ||
| </div> | ||
| <div class="flex items-center justify-center gap-1"> | ||
| <img src=${goldCoinIcon} width="13" height="13" /> | ||
| <span class="text-yellow-300" | ||
| >${renderNumber(this.cost(this._hoveredUnit))}</span | ||
| > | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Anchor the tooltip to the hovered unit.
This tooltip is pinned with fixed top-[22rem] left-4, so it will drift away from the unit bar when the control panel moves between classic/split layouts or when the viewport height changes. Please position it from the hovered item or the panel container instead of hard-coding viewport coordinates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/UnitDisplay.ts` around lines 142 - 160, The
tooltip is currently positioned with hard-coded viewport coordinates (CSS
classes fixed top-[22rem] left-4) so it drifts; in UnitDisplay.ts update the
tooltip rendering to anchor to the hovered element or panel container instead:
remove the fixed/top/left classes and compute position using the hovered item's
DOM bounds (use the element that corresponds to this._hoveredUnit or the panel
container), set inline style top/left (or use absolute positioning within a
relatively positioned container) when rendering the tooltip that shows
this._hoveredStructureKey / this._hoveredDisplayHotkey /
renderNumber(this.cost(this._hoveredUnit)) with goldCoinIcon; ensure fallback
positioning if bounds are unavailable and recalc on layout changes or window
resize.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
This PR isolates the in-game control panel work (split/classic panel variants) and related positioning/visual updates for attacks and unit bar integration.
Included
index.html-The color of the regeneration bar depends on whether you are in optimal regeneration, ranging from red (not enough troops) to green optimal and then to blue (too many troops).
-New information tells us how much gold was generated in the last minute
-The attacks are displayed in such a way that even if you spam them, they don't cover a large part of the screen.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
HulKiora
--- Screenshot ---
Layou Split - Panel Split :
Layout Split - Panel Classic :
Layout Classic - Panel Split
Layout Classic - Panel Classic :