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

issue #218: add russian translations #245

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

Conversation

lizzyaustad
Copy link

@lizzyaustad lizzyaustad commented Sep 13, 2020

What

Add support for Russian translations

Why

Ticket #218

Ensure the following interactions work as expected. Please test using a mobile form factor.

  • Tapping on a marker on the map displays information about the marker in a popup.
  • Tapping the "Show list of locations" button replaces the map view with a list view.
  • Tapping an item in the list replaces the list view with a map view, and navigates the map to the tapped item on the map.
  • Tapping one of the ✅'s in the legend filters items on the map and in the list.
  • Changing the language to spanish changes things on the page.
  • Clicking the Help/Info button opens and closes a menu with information.
  • Clicking the X Close button on the Help/Info screen closes the modal.

Check the app in the following web browsers:

  • Chrome
  • Firefox
  • Edge
  • Safari

Copy link
Contributor

@amaxama amaxama left a comment

Choose a reason for hiding this comment

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

Thanks for taking this up!!

I made one tiny comment, and then we also have moved away from using flags, so there's no need to add the lang-rus.png ... I know the LANGUAGE_TRANSLATION.md` still says to add a flag image so if you wanted you could also remove that step from that file :)

@lizzyaustad lizzyaustad force-pushed the 218_russian_translation branch 2 times, most recently from da54a69 to 9a7935d Compare September 13, 2020 21:07
@lizzyaustad
Copy link
Author

@amaxama thanks! I removed the image file and the image references in the new language instructions. Let me know if there were any other issues with the PR I missed

docs/LANGUAGE_TRANSLATION.md Outdated Show resolved Hide resolved
@@ -145,7 +131,8 @@ constructor() {
orm,
oji,
dak,
vie
vie,
ru
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we've mostly been using three-letter codes for languages, could change this to rus or another three-letter code?

src/js/translator.js Outdated Show resolved Hide resolved
@amaxama amaxama linked an issue Sep 17, 2020 that may be closed by this pull request
Copy link
Contributor

@amaxama amaxama left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this @lizzyaustad ! I added a couple comments for things I didn't catch the first time around.

"moment_d": "день",
"moment_M": "месец",
"moment_y": "год"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We've had a few more values added to the translation spreadsheet, would you mind adding those here?

#### Moment.js

We use [`moment.js`](https://momentjs.com/) to display text based on time computations ("an hour ago", "a few days ago", "in 3 days"). Some languages are already translated and supported by default. For others, we have created a translation file that will work with `moment.js` which can be found under [`src/locale`](https://github.com/Twin-Cities-Mutual-Aid/twin-cities-aid-distribution-locations/tree/master/src/locale).

If the language is already supported by `moment.js`, the `locale` value in the `XXX.json` file should match the one that `moment.js` uses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you go over these steps to check whether moment.js supports russian or not?

@@ -154,8 +139,6 @@ constructor() {
#### Add language checklist

- [ ] Create a new translation file (`src/i18n/XXX.json`)
- [ ] Add a new flag image (`public/images/lang-XXX.png`)
- [ ] `locale` should match existing `moment.js` translation or new `moment.js` locale created (`src/locale/XXX.js`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this step for the locale is being removed?

@lizzyaustad
Copy link
Author

@amaxama it looks like my previous comment was removed, but I had mentioned that the moment.js translation did exist and was ru not rus. I asked if you still wanted the 3-letter country code instead of the 2-letter version that moment.js supports and if not, whether the instructions about using the moment code should be removed. I didn't hear back, so I guessed that you still wanted to use the 3 letter country code and that the instructions to use the moment code were no longer relevant. Can you clarify which code you would like me to use?

@amaxama
Copy link
Contributor

amaxama commented Oct 30, 2020

@lizzyaustad I don't think I ever saw your previous comments. Oh sorry, I was just saying to use 3 letters when naming the json file since that's what was in the language_translation file, not necessarily for the moment locale. It doesn't really matter that much about the file name, I was just thinking about consistency. Sorry if I was unclear before! But I think the moment stuff is still relevant.

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.

Russian translations ready to be added to map
2 participants