-
Notifications
You must be signed in to change notification settings - Fork 793
Show troop count of troop transport boats ⛵ #3056
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
WalkthroughAdds troop display for TransportShip units in the unit information UI overlay. Updates the naval invasion notification message to include the transported troop count alongside the attacker name. Both changes display existing data in new locations. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
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 |
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: 1
🤖 Fix all issues with AI agents
In `@src/core/execution/TransportShipExecution.ts`:
- Around line 116-120: Replace the hardcoded English message passed to
mg.displayIncomingUnit with a call to translateText using a translation key
(e.g. "events_display.naval_invasion_inbound") and pass attacker and troops as
params; specifically, in TransportShipExecution where mg.displayIncomingUnit is
invoked (using this.boat.id(), MessageType.NAVAL_INVASION_INBOUND,
this.target.id()), build the message via
translateText("events_display.naval_invasion_inbound", { attacker:
this.attacker.displayName(), troops: renderTroops(this.boat.troops()) }) and use
that instead of the raw string, and ensure you add the corresponding entry to
en.json.
🧹 Nitpick comments (1)
src/core/execution/TransportShipExecution.ts (1)
1-1: Move rendering utilities to shared folder to avoid core → client dependency.
renderTroopsis imported fromsrc/client/Utils.tsinto core execution code, creating a layering violation. The core layer should not depend on client layer utilities. This pattern affects multiple core files (PlayerImpl.ts, AttackExecution.ts, GameImpl.ts, and others also importrenderNumberandtranslateTextfrom client).Since these are pure formatting functions (e.g.,
renderTroopsdivides by 10 and formats for display), move them tosrc/core/Utils.tsor a dedicatedsrc/shared/folder so both core and client layers can use them without breaking layering rules.
| // TODO TranslateText | ||
| `Naval invasion incoming from ${this.attacker.displayName()}`, | ||
| `Naval invasion incoming from ${this.attacker.displayName()} (${renderTroops(this.boat.troops())})`, | ||
| MessageType.NAVAL_INVASION_INBOUND, | ||
| this.target.id(), | ||
| ); |
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.
Hardcoded message string bypasses translation system.
The TODO on line 116 notes this needs translation. The PR description says all text uses translateText(), but this message is a raw English string. Other calls in this file (like lines 64-70) use translation keys with parameter objects.
Consider using a pattern like:
mg.displayIncomingUnit(
this.boat.id(),
translateText("events_display.naval_invasion_inbound", {
attacker: this.attacker.displayName(),
troops: renderTroops(this.boat.troops()),
}),
MessageType.NAVAL_INVASION_INBOUND,
this.target.id(),
);This would need a corresponding entry in en.json.
🤖 Prompt for AI Agents
In `@src/core/execution/TransportShipExecution.ts` around lines 116 - 120, Replace
the hardcoded English message passed to mg.displayIncomingUnit with a call to
translateText using a translation key (e.g.
"events_display.naval_invasion_inbound") and pass attacker and troops as params;
specifically, in TransportShipExecution where mg.displayIncomingUnit is invoked
(using this.boat.id(), MessageType.NAVAL_INVASION_INBOUND, this.target.id()),
build the message via translateText("events_display.naval_invasion_inbound", {
attacker: this.attacker.displayName(), troops: renderTroops(this.boat.troops())
}) and use that instead of the raw string, and ensure you add the corresponding
entry to en.json.
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!
Description:
Troop count display for naval invasion message:
On hover, PlayerInfoOverlay shows the troop count now:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FloPinguin