Add PlayerInfoModal #2058
Add PlayerInfoModal #2058
Conversation
WalkthroughAdds a PlayerInfoModal component, integrates it into the client UI and Main wiring, and updates English translations by adding Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Main as Client Main
participant PIM as PlayerInfoModal
participant API as Server API
User->>Main: Click "player-info-button"
Main->>PIM: open()
Note right of PIM #DDEBF7: Modal visible
Main->>PIM: onUserMe(userMeResponse)
alt logged in (has publicId)
PIM->>API: fetchPlayerById(publicId)
API-->>PIM: player { statsTree, recentGames } or error
alt data loaded
PIM-->>User: Render stats & recent games
else load error / missing data
PIM-->>User: Show `player_modal.error.load`
end
else not logged in
PIM-->>User: Clear data / idle
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 4
🧹 Nitpick comments (7)
src/client/index.html (1)
217-223: Button: add a11y + correct/localized labeling, and prevent implicit submit.Set type="button", add aria-label, and drop the unclear title.
- <button - class="w-10 h-10 min-w-[2.5rem] min-h-[2.5rem] p-0 bg-[#007bff] hover:bg-[#0056b3] text-white rounded-lg border-none cursor-pointer transition-colors duration-300 flex items-center justify-center" - title="player-info" - id="player-info-button" - > + <button + id="player-info-button" + type="button" + aria-label="Player Info" + class="w-10 h-10 min-w-[2.5rem] min-h-[2.5rem] p-0 bg-[#007bff] hover:bg-[#0056b3] text-white rounded-lg border-none cursor-pointer transition-colors duration-300 flex items-center justify-center" + > ⚙️ </button>src/client/Main.ts (3)
323-323: Avoid NPE if modal not found.- this.playerInfoModal.onUserMe(null); + this.playerInfoModal?.onUserMe(null);
331-331: Avoid NPE if modal not found.- this.playerInfoModal.onUserMe(userMeResponse); + this.playerInfoModal?.onUserMe(userMeResponse);
529-569: Hide new UI on game start and close the modal like others.Follow existing pattern: hide the button and include the modal in the batch-close list.
console.log("Closing modals"); document.getElementById("settings-button")?.classList.add("hidden"); + document.getElementById("player-info-button")?.classList.add("hidden"); document .getElementById("username-validation-error") ?.classList.add("hidden"); [ "single-player-modal", "host-lobby-modal", "join-private-lobby-modal", "game-starting-modal", "game-top-bar", "help-modal", "user-setting", "territory-patterns-modal", + "player-info-modal", "language-modal", "news-modal", "flag-input-modal", "account-button", "token-login", ].forEach((tag) => {src/client/PlayerInfoModal.ts (3)
45-50: Wait for first render before opening.Prevents a race where modalEl isn’t available yet.
- public open() { - this.loadError = null; - this.requestUpdate(); - this.modalEl?.open(); - } + public async open() { + this.loadError = null; + await this.updateComplete; + this.modalEl?.open(); + }
55-67: Minor: unused warningMessage.Either render it or remove for now.
69-89: Drop extra requestUpdate calls; Lit will re-render via @State.Also consider guarding against stale responses if multiple loads race (optional).
const data = await fetchPlayerById(playerId); if (!data) { this.loadError = "player_modal.error.load"; - this.requestUpdate(); return; } this.recentGames = data.games; this.statsTree = data.stats; - this.requestUpdate(); } catch (err) { console.warn("Failed to load player data:", err); this.loadError = "player_modal.error.load"; - this.requestUpdate(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/Main.ts(4 hunks)src/client/PlayerInfoModal.ts(1 hunks)src/client/index.html(2 hunks)
🧰 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/index.htmlsrc/client/Main.tssrc/client/PlayerInfoModal.ts
🧬 Code graph analysis (1)
src/client/PlayerInfoModal.ts (2)
src/core/ApiSchemas.ts (3)
UserMeResponse(56-56)PlayerStatsTree(73-73)PlayerGame(84-84)src/client/jwt.ts (1)
fetchPlayerById(274-311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Has Milestone
- GitHub Check: Validate Description
🔇 Additional comments (3)
src/client/index.html (1)
421-421: LGTM: host element added.Adding here is consistent with other modal hosts.
src/client/Main.ts (2)
25-25: LGTM: import.
95-95: LGTM: field addition.
src/client/index.html
Outdated
| <button | ||
| class="w-10 h-10 min-w-[2.5rem] min-h-[2.5rem] p-0 bg-[#007bff] hover:bg-[#0056b3] text-white rounded-lg border-none cursor-pointer transition-colors duration-300 flex items-center justify-center" | ||
| title="player-info" | ||
| id="player-info-button" | ||
| > | ||
| ⚙️ | ||
| </button> |
There was a problem hiding this comment.
I think we should reuse login/user button on the top right corner.
There was a problem hiding this comment.
I was hoping that this modal could eventually be used to view other players’ data as well — would that be acceptable?
As for the one in the upper right, I had assumed it was meant for viewing one’s own data (statistics).
There was a problem hiding this comment.
Additional note:
Regarding “I think we should reuse the login/user button on the top right corner.”
Does this mean moving the button that opens the PlayerInfoModal into the AccountModal,
or does it mean deleting the PlayerInfoModal entirely and having the AccountModal open the status directly instead?
There was a problem hiding this comment.
yeah maybe integrating the login modal & stats modal into the same modal.
There was a problem hiding this comment.
Regarding “I think we should reuse the login/user button on the top right corner.”
Does this mean moving the button that opens the PlayerInfoModal into the AccountModal,
or deleting the PlayerInfoModal entirely and having the AccountModal open the status directly instead?
Also, for this PR, should I proceed as it is now, or would you prefer me to adjust it to reflect this integration?
|
Each status (for example, whether the most recent game was won) will be submitted in separate PRs, so they are not included in this one. new button (open player-info-modal) |
src/client/PlayerInfoModal.ts
Outdated
There was a problem hiding this comment.
I think this should be part of the AccountModal. maybe a separate tab at the top?
Description:
This PR only adds the modal for displaying player statistics.
The UI will be added in the next PR; for now, this includes only the fetching and data organization parts.
Once this is merged, I will create a follow-up PR to display each individual status (such as the list of recently played games) within this modal.
(origin pr:#1758)
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aotumuri