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

Feature localization #434

Closed
wants to merge 2 commits into from
Closed

Feature localization #434

wants to merge 2 commits into from

Conversation

Platiplus
Copy link

What does this do?

Proof of concept about localization of the database

How was it tested?

run node ./scripts/createLanguageFiles.js

Is there a Github issue this is resolving?

#81

Did you update the docs in the API? Please link an associated PR if applicable.

<It's not clear if I don't update this text with relevant info>

Here's a fun image for your troubles

random photo - update me

Copy link
Collaborator

@bagelbits bagelbits left a comment

Choose a reason for hiding this comment

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

A couple of comments to start. I'm looking forward to talking to you about this more tomorrow.

Comment on lines 5 to +7
"desc": [
"You can use a weapon that has the ammunition property to make a ranged attack only if you have ammunition to fire from the weapon. Each time you attack with the weapon, you expend one piece of ammunition. Drawing the ammunition from a quiver, case, or other container is part of the attack (you need a free hand to load a one-handed weapon).",
"At the end of the battle, you can recover half your expended ammunition by taking a minute to search the battlefield. If you use a weapon that has the ammunition property to make a melee attack, you treat the weapon as an improvised weapon (see \"Improvised Weapons\" later in the section). A sling must be loaded to deal any damage when used in this way."
"ammunitionDesc1",
"ammunitionDesc2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make more sense for ammunitionDesc to be a list instead?

Copy link
Author

Choose a reason for hiding this comment

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

Yes and No. That's one of my concerns about this approach instead of just mimicking the whole file structure and start translating it. We'll talk more today.

"finesseDesc1": "When making an attack with a finesse weapon, you use your choice of your Strength or Dexterity modifier for the attack and damage rolls. You must use the same modifier for both rolls.",
"heavyName": "Heavy",
"heavyDesc1": "Small creatures have disadvantage on attack rolls with heavy weapons. A heavy weapon's size and bulk make it too large for a Small creature to use effectively.",
"lighName": "Light",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be lightName instead of lighName?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Freaking typo haha

@@ -0,0 +1 @@
*.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we add these to the git ignores?

Copy link
Author

Choose a reason for hiding this comment

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

Just added this gitignore so we would't commit the translated files by accident when messing around with the script. Also PERSONALLY I think that this keeps the folder created for us on git, so we shouldn't fall on some edge cases when we have to account for cache and checking if the file is already there or not in the script, I had some problems with this in the past, but is totally optional.

@Platiplus Platiplus closed this Dec 29, 2021
@Platiplus Platiplus deleted the feature-localization branch December 29, 2021 16:17
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.

2 participants