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

Customizable Golden Age label #13111

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

k-oa
Copy link
Contributor

@k-oa k-oa commented Mar 30, 2025

image

My biggest PR so far. Please destroy my code so i can grow as a programmer

@SomeTroglodyte
Copy link
Collaborator

destroy my code

Sorry, must question the concept first. Modders can already replace all this via mod-specific translation files, including English.properties.

@k-oa
Copy link
Contributor Author

k-oa commented Mar 30, 2025

Modders can already replace all this via mod-specific translation files

Yes but not for one specific Civ.
http://www.megabearsfan.net/image.axd/2013/12/CivV-Carnival_musician_590x369_opt.jpg

@SomeTroglodyte
Copy link
Collaborator

but not for one specific Civ

Got it. Still, a lot of effort for a tiny little visual...

Sooo...

  • var enteredGoldenAge = "You have entered a Golden Age" more data - really necessary? I think this could get away with just a "You enter a [Golden Age]" template and giving it the goldenAgeName in code. Maybe.
  • getErrorSeverity - idea: Restrict length in (1..?)?
  • DeprecationLevel.ERROR - well done to deprecate first (was looking for this from the moment I typed "sooo"), but convention is to give modders a few µs time to update!
  • val nation = viewingCiv.nation in AlertPopup - why not val goldenAgeName = viewingCiv.nation.goldenAgeName if you're not even using it in the two candidate lines?
  • uppercase() - makes me wonder. Could we leave this more open? Would non-latin languages profit? Different casing conventions or those not even using cases or those the std::libs so far ignore? Or more crass: uppercase before tr() so both lines Golden Age = and GOLDEN AGE = will need translation?
  • template.properties untouched -> TFW would pick up Uniques and UniqueParameter instances automatically IIRC, so - are there obsolete entries?

That's as much as I can see without opening another Studio instance...

@k-oa
Copy link
Contributor Author

k-oa commented Mar 30, 2025

  • var enteredGoldenAge = "You have entered a Golden Age" more data - really necessary? I think this could get away with just a "You enter a [Golden Age]" template and giving it the goldenAgeName in code. Maybe.

I was thinking about conditions that you can't "enter" but that "arrive" or something like that.

But I agree. It isn't that necessary.

@k-oa
Copy link
Contributor Author

k-oa commented Mar 30, 2025

  • DeprecationLevel.ERROR - well done to deprecate first (was looking for this from the moment I typed "sooo"), but convention is to give modders a few µs time to update!

So ... DeprecationLevel.WARNING?

@SomeTroglodyte
Copy link
Collaborator

DeprecationLevel.WARNING

Meaning the players will see the mod "denigrated" on their new game screen right after getting the version with this PR. There was a warning for Options only so it would show only in modchecker... Maybe inspect git blame for how it was done in the past? Or Yair tells you.

Comment on lines +42 to +43
"goldenAgeName": { "type": "string" },
"enteredGoldenAge": { "type": "string" },
Copy link
Owner

Choose a reason for hiding this comment

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

We're against adding extra fields when things can be one via a Unique, so the correct answer here is to add a unique e.g. "Golden Age is called [comment]"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants