add colored borders to player's quickchat option buttons #2821
Conversation
|
|
WalkthroughA styling change adds a 2px border to player option buttons in the Quick-Chat (ChatModal), using each player's territory color converted to hex. No control flow or data handling changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-06-09T02:20:43.637ZApplied to files:
📚 Learning: 2025-06-22T05:48:19.241ZApplied to files:
📚 Learning: 2025-06-10T09:56:44.473ZApplied to files:
🧬 Code graph analysis (1)src/client/graphics/layers/ChatModal.ts (1)
⏰ 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)
🔇 Additional comments (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. 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
🤖 Fix all issues with AI agents
In @src/client/graphics/layers/ChatModal.ts:
- Around line 145-147: Collapse the multi-line style attribute in ChatModal.ts
into a single properly formatted style attribute that contains the template
expression using player.territoryColor().toHex(), then run the formatter; after
editing the style attribute (the block containing
player.territoryColor().toHex()) run npx prettier --write
src/client/graphics/layers/ChatModal.ts to fix remaining formatting issues.
🧹 Nitpick comments (2)
src/client/graphics/layers/ChatModal.ts (2)
145-147: Consider using CSS custom properties instead of inline styles.Inline styles make the template harder to maintain. You could set the border color as a CSS custom property (CSS variable) and keep the styling in CSS:
style="--player-border-color: ${player.territoryColor().toHex()};"Then in your CSS:
.chat-option-button { border: 2px solid var(--player-border-color, transparent); }This separates styling concerns from the template and makes the code cleaner.
145-147: Consider accessibility for colorblind usersThe colored border alone doesn't fully convey player information. While the player name is displayed as text, adding a visual indicator alongside the color would improve accessibility for colorblind users and better align with WCAG Success Criterion 1.4.1. Consider:
- Player initials in a colored circle
- A small icon or badge next to the player name
- A pattern or texture in addition to the border color
This makes identification clearer for all users, especially those with color-vision deficiencies.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/ChatModal.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/Colors.ts:114-330
Timestamp: 2025-06-06T15:36:31.739Z
Learning: In the OpenFrontIO project, color improvements are implemented incrementally. The current focus for player colors is ensuring sufficient unique color availability rather than optimizing for visual distinction or accessibility features.
📚 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/graphics/layers/ChatModal.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/ChatModal.ts (1)
src/core/game/GameImpl.ts (1)
player(483-489)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/ChatModal.ts
[warning] 1-1: Code style issues found in the above file. Run 'npx prettier --write' to fix.
🔇 Additional comments (1)
src/client/graphics/layers/ChatModal.ts (1)
145-147: All API methods exist and are correctly used.
PlayerView.territoryColor()returns aColordobject from the "colord" library, which provides the.toHex()method for converting colors to hex string format. The code chain is valid.
| style="border: 2px solid ${player | ||
| .territoryColor() | ||
| .toHex()};" |
There was a problem hiding this comment.
Fix the code formatting.
The pipeline reports a Prettier formatting issue. Run npx prettier --write src/client/graphics/layers/ChatModal.ts to fix the formatting of the multi-line style attribute.
🤖 Prompt for AI Agents
In @src/client/graphics/layers/ChatModal.ts around lines 145 - 147, Collapse the
multi-line style attribute in ChatModal.ts into a single properly formatted
style attribute that contains the template expression using
player.territoryColor().toHex(), then run the formatter; after editing the style
attribute (the block containing player.territoryColor().toHex()) run npx
prettier --write src/client/graphics/layers/ChatModal.ts to fix remaining
formatting issues.
DenelDuck
changed the title
|
My concern with this is for colourblind people with accessability issues, this could be a slightly weird feature as it would just assign them a weird grey colour (depending on their colour blind issue, i know) |
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
|
As far as I know most colourblind issues lie in red-green and blue-yellow contrast. The contrast between the coloured border and the gray/white button should not have a big impact on colourblindness (only 1 out of 30.000 see only shades of gray). Besides, if in the near future Openfront supports colourblind features like different colour palettes for the territory colours, this feature would adopt this automatically, as it refers to each player's territory colour. |
|
I think this is pretty decent justification. @DenelDuck |
4 participants
Resolves #2820
Description:
Add colored borders to the
chat-option-buttoncorresponding to each players territory color.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
denelduck