Skip to content
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

Added branch to EndRound method and unit to round-end-system.ftl #33304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

freeagent1384
Copy link

@freeagent1384 freeagent1384 commented Nov 14, 2024

About the PR

Addressing issue " small typo in the round restart chat alert #9797"

Why / Balance

This is my first contribution to an open source project so please be gentle. This seems like a frivolous change but it was accepted as an open issue so I decided to address it. I had to moc some values as I could not figure out how to get the restartround time under 120 seconds but I believe it will work if this turns out to be the case.

Technical details

I really just added an if condition to the existing EndRound method.

Media

Requirements

Breaking changes

I don't believe there should be any breaking changes for the English version but I'm an ignorant American and have no idea how to update the round-end-system.ftl file for other languages.

Personal Note

Please let me know how I can name PR's better or associate the PR to an open issue or any other advice you can give. I'm hear to practice good code, not a gamer actually.

@SlamBamActionman SlamBamActionman added the S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. label Nov 14, 2024
@SaphireLattice SaphireLattice added T: Bugfix Type: Bugs and/or bugfixes Changes: UI Changes: Might require knowledge of UI design or code. S: Needs Review Status: Requires additional reviews before being fully accepted DB: Beginner Friendly Difficulty: Great for beginners. Unambiguous in scope, and explains how to achieve the result. T: UI / UX Improvement Type: UI and player facing interactive graphical interfaces P: Very Low / Fluff size/XS Denotes a PR that changes 0-9 lines. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 15, 2024
Copy link
Contributor

@SaphireLattice SaphireLattice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly this is kinda an area of localisation. As in, the numbers should have some kind of formatter/whatever that lets you pick different forms of word for different numbers.

E.g. Russian requires specific words for groups of: ending on 1, ending on 2-4, ending on 5-9 or it's a number 10-20 (inclusive). (Er, might be missing some nuance, but yeah)
English is, afaik, just "Is it exactly 1? Singular. Otherwise plural"

Which we do actually have support for!

Comment on lines 8 to 12
round-end-system-round-restart-eta-announcement = Restarting the round in {$time} {$units}...

eta-units-minute = minute
eta-units-minutes = minutes
eta-units-seconds = seconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
round-end-system-round-restart-eta-announcement = Restarting the round in {$time} {$units}...
eta-units-minute = minute
eta-units-minutes = minutes
eta-units-seconds = seconds
round-end-system-round-restart-eta-announcement = Restarting the round in {$time} {$units}...
eta-units-minutes = {$amount ->
[one] minute
*[other] minutes
}
eta-units-seconds = {$amount ->
[one] second
*[other] seconds
}

Note that this will require the units's Loc.GetString(unitsLocString) to supply time as well. Ideally you would change it all to be a bit better somehow, e.g. have the time string generated separately and just substituted into the "restarting the round..."

Also this sorta "minutes" and "seconds" stuff probably should be moved into being its own thing somewhere and have this just reference it somehow. But dunno if this number stuff happens often enough to warrant it.

@SaphireLattice SaphireLattice added D3: Low Difficulty: Some codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. and removed DB: Beginner Friendly Difficulty: Great for beginners. Unambiguous in scope, and explains how to achieve the result. P: Very Low / Fluff labels Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: UI Changes: Might require knowledge of UI design or code. D3: Low Difficulty: Some codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Needs Review Status: Requires additional reviews before being fully accepted size/XS Denotes a PR that changes 0-9 lines. T: Bugfix Type: Bugs and/or bugfixes T: UI / UX Improvement Type: UI and player facing interactive graphical interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants