* update leaderboard align #1189
Conversation
WalkthroughThis update removes the separate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GameLeftSidebar
participant Leaderboard
participant TeamStats
User->>GameLeftSidebar: View sidebar
GameLeftSidebar->>GameLeftSidebar: Update player team label visibility and color
GameLeftSidebar->>Leaderboard: Render leaderboard inside flex container
GameLeftSidebar->>TeamStats: Render team stats with responsive margin
Leaderboard->>User: Toggle top 5/all players (button click)
User->>Leaderboard: Click toggle button
Leaderboard->>Leaderboard: Update view accordingly
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/client/graphics/layers/Leaderboard.ts (1)
184-191:⚠️ Potential issueTranslate the toggle-button label
Raw strings
"Show All"/"Show Top 5"bypasstranslateText(), which contradicts the PR policy that all user-facing text must be localised. This also makes the UI harder to internationalise later.- ${this.showTopFive ? "Show All" : "Show Top 5"} + ${this.showTopFive + ? translateText("leaderboard.showAll") + : translateText("leaderboard.showTopFive")}
🧹 Nitpick comments (1)
src/client/graphics/layers/GameLeftSidebar.ts (1)
67-73: Flex wrapper – double-check class mix
block lg:flex flex-wrapis fine, butflex-wrapdoes nothing while the element isdisplay:blockon small screens. Considerflex lg:flexor droppingflex-wrapuntillg.Also, adding
flex 1to<team-stats>host element works only if its host isdisplay: flex(it defaults toinline). Addblockorflexto the host style if width issues appear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/layers/GameLeftSidebar.ts(2 hunks)src/client/graphics/layers/Leaderboard.ts(1 hunks)
🔇 Additional comments (2)
src/client/graphics/layers/Leaderboard.ts (1)
184-185: Padding change looks goodReducing
md:pxfrom3to2.5keeps the button aligned with the new sidebar spacing. No functional impact spotted.src/client/graphics/layers/GameLeftSidebar.ts (1)
44-46: Validate top/height combo
top-[50px]withmax-h-[calc(100vh-80px)]leaves a 30 px gap at the bottom (50 px + 100vh − 80 px = 100vh − 30 px). If the intent was to occupy the full viewport minus header height, adjust one of the values, e.g.max-h-[calc(100vh-50px)]. Please confirm on all breakpoints.
|
As discussed on discord, this contains an improvement (teams leaderboard next to player leaderboard on non-mobile, while on mobile still stacked on top of each other). If it could move up a little more to the top of the screen (where 'Your team: team x' is at the start of a Teams game) then it would be perfect imo. https://discord.com/channels/1359946986937258015/1381293863712591872/1383871726412370013 |
Check it now please |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/client/graphics/layers/GameLeftSidebar.ts (1)
66-68: Getter may yieldundefined, break strictness
game?.config()short-circuits toundefined, so the caller must handle a union type. Coerce to boolean to keep the API simple:- private get isTeamGame(): boolean { - return this.game?.config().gameConfig().gameMode === GameMode.Team; - } + private get isTeamGame(): boolean { + return !!( + this.game && this.game.config().gameConfig().gameMode === GameMode.Team + ); + }
🧹 Nitpick comments (4)
src/client/graphics/layers/GameLeftSidebar.ts (4)
19-23: Avoidnew Colord– prefer the factory for clearer, idiomatic usage
colord("#FFFFFF")is the canonical way to create a color instance; the constructor is mostly kept for legacy reasons. Swapping to the factory avoids potential TS type-narrowing issues and reads cleaner.-import { Colord } from "colord"; +import { colord, Colord } from "colord"; ... - private playerColor: Colord = new Colord("#FFFFFF"); + private playerColor: Colord = colord("#FFFFFF");
31-34: GuardisPlayerTeamLabelVisiblebehind a strict boolean check
isTeamGamecan returnundefined(see getter below). Using it directly in anifrelies on truthiness and leaks the union type. Either coerce with!!or tighten the getter (see next comment).
77-89: Add semantic role for better accessibility
The label is plain<div>; give screen-readers a hook.- <div + <div + role="status"Small change, big gain for a11y.
90-96: Duplicate spacing utilities – pick one
gap-2already handles spacing;space-x-2is redundant and slightly heavier in the DOM.-<div class="flex items-center gap-2 space-x-2 text-white mb-2"> +<div class="flex items-center gap-2 text-white mb-2">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/client/graphics/GameRenderer.ts(0 hunks)src/client/graphics/layers/ControlPanel.ts(1 hunks)src/client/graphics/layers/GameLeftSidebar.ts(5 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(1 hunks)src/client/graphics/layers/PlayerTeamLabel.ts(0 hunks)src/client/graphics/layers/TopBar.ts(1 hunks)src/client/index.html(0 hunks)
💤 Files with no reviewable changes (3)
- src/client/index.html
- src/client/graphics/GameRenderer.ts
- src/client/graphics/layers/PlayerTeamLabel.ts
✅ Files skipped from review due to trivial changes (3)
- src/client/graphics/layers/ControlPanel.ts
- src/client/graphics/layers/PlayerInfoOverlay.ts
- src/client/graphics/layers/TopBar.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
## Description: This fix issue of leaderboard overlaping other elements. Also give option to dispaly ads below leaderboard since on desktop they are next to each other ## Please complete the following: - [ ] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [ ] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced - [x] I understand that submitting code with bugs that could have been caught through manual testing blocks releases and new features for all contributors ## Please put your Discord username so you can be contacted if a bug or regression is found: Diessel   ts/assets/5886ceb2-2d15-4b0e-9c30-8c61b0255f48)   --------- Co-authored-by: evanpelle <evanpelle@gmail.com>
This fix issue of leaderboard overlaping other elements. Also give option to dispaly ads below leaderboard since on desktop they are next to each other - [ ] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [ ] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced - [x] I understand that submitting code with bugs that could have been caught through manual testing blocks releases and new features for all contributors regression is found: Diessel   ts/assets/5886ceb2-2d15-4b0e-9c30-8c61b0255f48)   --------- Co-authored-by: evanpelle <evanpelle@gmail.com>
3 participants
Description:
This fix issue of leaderboard overlaping other elements.
Also give option to dispaly ads below leaderboard since on desktop they are next to each other
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Diessel
ts/assets/5886ceb2-2d15-4b0e-9c30-8c61b0255f48)