-
Notifications
You must be signed in to change notification settings - Fork 707
Alert frame: add to in-game settings, orange for attack instead of red #2561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Updated the alert frame description to include attack over land
Orange is well discernable from red for colorblind, which isn't the case for yellow, and also is still an alarming color
Previously only existed in UserSettingsModal on the homepage
WalkthroughExtended AlertFrame to support two alert types—betrayal and land-attack—each with distinct border styling. Updated SettingsModal with a toggle control to manage alert frame display. Refreshed localization strings to reflect the expanded alert behavior. Changes
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/AlertFrame.ts (1)
24-25: Alert-type handling and styling look solid
alertType: "betrayal" | "land-attack"is a nice, tight union and keeps the set of alert kinds explicit.- CSS split into a base
.alert-border+ per-type classes (.betrayal,.land-attack) is simple and composable.- Logic correctly sets
alertType = "betrayal"on betrayal updates and"land-attack"on qualifying incoming attacks, right beforeactivateAlert(), so the right color is always used.- Render binding
class=${\alert-border animate ${this.alertType}`}` is straightforward and idiomatic for Lit.Optional: if you expect more alert types later, you could introduce a small alias like:
type AlertType = "betrayal" | "land-attack";and reuse it for the state and any helpers, to keep the union in one place.
Also applies to: 41-42, 47-53, 132-133, 216-217, 252-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/SirenIconWhite.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
resources/lang/en.json(1 hunks)src/client/graphics/layers/AlertFrame.ts(5 hunks)src/client/graphics/layers/SettingsModal.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/graphics/layers/SettingsModal.ts (1)
src/client/LangSelector.ts (1)
translateText(258-278)
🔇 Additional comments (2)
resources/lang/en.json (1)
339-339: Alert-frame description matches new behaviorText is clear, simple, and correctly mentions both betrayal and land attacks. No issues.
src/client/graphics/layers/SettingsModal.ts (1)
11-11: Alert-frame toggle wiring is consistent and clear
- Using a dedicated handler that calls
userSettings.toggleAlertFrame()andrequestUpdate()matches the pattern of existing toggles.- New button mirrors the structure of other settings (icon, label, description, On/Off text via
translateText), so UX stays consistent and easy to understand.- The
alertFrameboolean accessor and toggle keep things simple; no extra complexity.No changes needed here.
Also applies to: 134-137, 355-373
evanpelle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
openfrontio#2561) ## Description: The red alert frame for betrayals was added in openfrontio#1195. It also flashes red for incoming land attacks since openfrontio#2358. The same color for betrayals and attacks leads to confusion. And possibly red alert fatigue. But when players find themselves fatigued and want to shut it off for awhile, they can't because the setting doesn't exist in-game. Also, the setting description on the homepage settings didn't yet reflect that the alert frame flashes for attacks too. This PR fixes this by: - making the color for land attacks orange. This is well discernable from red for various colorblindness types, while still looking alarming. - adding the setting to in-game SettingsModal - adding land attack to setting description Reference to comments on it on Dev Discord: https://discord.com/channels/1359946986937258015/1381347989464809664/1441232666065240064 https://discord.com/channels/1359946986937258015/1360078040222142564/1434574256704061523 Orange alert frame on being attacked over land: https://github.com/user-attachments/assets/e0772d62-5b25-4213-a393-dd5af13e8bc9 Settings description change and addition to in-game toggles: <img width="560" height="160" alt="Added to description what was added in PR 2358" src="https://github.com/user-attachments/assets/bc6e2206-b7ac-498d-9009-d2b6e302d3cf" /> <img width="665" height="425" alt="In SettingsModal and with attacks added to description" src="https://github.com/user-attachments/assets/d489830c-e359-4a5f-8eb4-3caa7d0c21b2" /> ## Please complete the following: - [x] 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 - [x] 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 ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
Description:
The red alert frame for betrayals was added in #1195. It also flashes red for incoming land attacks since #2358.
The same color for betrayals and attacks leads to confusion. And possibly red alert fatigue. But when players find themselves fatigued and want to shut it off for awhile, they can't because the setting doesn't exist in-game. Also, the setting description on the homepage settings didn't yet reflect that the alert frame flashes for attacks too.
This PR fixes this by:
Reference to comments on it on Dev Discord:
https://discord.com/channels/1359946986937258015/1381347989464809664/1441232666065240064
https://discord.com/channels/1359946986937258015/1360078040222142564/1434574256704061523
Orange alert frame on being attacked over land:
https://github.com/user-attachments/assets/e0772d62-5b25-4213-a393-dd5af13e8bc9
Settings description change and addition to in-game toggles:

Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33