Skip to content

City state resources #4755

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

Merged
merged 105 commits into from
Aug 6, 2021
Merged

City state resources #4755

merged 105 commits into from
Aug 6, 2021

Conversation

MorphicDreamer
Copy link
Contributor

This is basically my old pull request but better!

@SomeTroglodyte
Copy link
Collaborator

Just remember you got nudged in that direction from someone who managed to overdo their Kdoc's. Also - there's tricks and quirks. Maybe I kept records of some stuff I learned in my notes, or maybe I forgot.

@MorphicDreamer
Copy link
Contributor Author

@SomeTroglodyte

I made the function more generic and moved the more specific stuff to the declaration of the list.

Thanks for the feedback.

@SomeTroglodyte
Copy link
Collaborator

<facepalm/> That Civ5 screenshot that could show Yair this feature was indeed in Civ5 - I searched in vain in issues - and it's of course in the predecessor PR, and of course thanks to ravignir:
#4752 (comment)
image

@SomeTroglodyte
Copy link
Collaborator

Forgot one nitpick: isn't .map {it} a no-op? As always, just food for thought.

@MorphicDreamer
Copy link
Contributor Author

@yairm210 gotcha will fix tomorrow morning

@MorphicDreamer
Copy link
Contributor Author

not sure how to use templates though

@yairm210
Copy link
Owner

yairm210 commented Aug 6, 2021

For the translation, we need

Build [improvementName] on [resourceName] (200 Gold) =
Gift Improvement =

to be added to the template.properties file

@ajustsomebody
Copy link
Contributor

For the translation, we need

Build [improvementName] on [resourceName] (200 Gold) =
Gift Improvement =

to be added to the template.properties file

what about just adding [amount] Gold?

@MorphicDreamer
Copy link
Contributor Author

@yairm210 i did it

@yairm210
Copy link
Owner

yairm210 commented Aug 6, 2021

looks good!
You have conflicts, resolve those and we'll merge :)

@MorphicDreamer
Copy link
Contributor Author

yep fixed the conflicts. Ready to merge. :)

@yairm210
Copy link
Owner

yairm210 commented Aug 6, 2021

The tests show that your translation lines don't end with a space
Add and the tests should pass :)

@MorphicDreamer
Copy link
Contributor Author

Fixed!

@yairm210 yairm210 merged commit 2a33958 into yairm210:master Aug 6, 2021
@MorphicDreamer MorphicDreamer deleted the CityStateResources branch August 6, 2021 13:17
@SomeTroglodyte
Copy link
Collaborator

Congrats on your second merged PR!

The short discussion on template.properties has prompted me to add an extra chapter to the wiki entry on translations - please tell me if I explained badly or where I missed a detail.

@MorphicDreamer
Copy link
Contributor Author

@SomeTroglodyte Thanks! The documentation looks good

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.

4 participants