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

Internationalize plan #42

Open
jinto opened this Issue Apr 6, 2019 · 34 comments

Comments

Projects
None yet
8 participants
@jinto
Copy link

jinto commented Apr 6, 2019

Hi,
I’m a Korean programmer and had CBT before. I think it should be great if you support other languages.

I translated ‘groovy programming language’ into korean and contributed translation of ‘django girls’.

Do you have plan for internalization?

@scaramagus

This comment has been minimized.

Copy link
Contributor

scaramagus commented Apr 6, 2019

Yes! I'd love to help with the Spanish translation!

@rogeriomarques

This comment has been minimized.

Copy link

rogeriomarques commented Apr 6, 2019

I'd love to help with portuguese and french (bilingual here)

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 6, 2019

Yes maybe? I would very much like to translate Quirk, but I one fear.

Let’s say we translate it to Korean. Then I want to add a new feature that requires text. Can we fall back to English in the meantime before it gets translated? Is that alright?

If so, then yeah. Let’s make translation the next big feature 🙌

@kwierbol

This comment has been minimized.

Copy link

kwierbol commented Apr 6, 2019

Polish speaker who would love to contribute to translation here! 🙌

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 6, 2019

Would love to have a Chinese localized version of the app, as well. Happy to contribute

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 6, 2019

Actually, it looks like there are some localized strings already? https://github.com/Flaque/quirk/search?q=auto_thought&unscoped_q=auto_thought

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 6, 2019

@briankung As of a couple minutes ago yes! We're gonna move the whole thing into an en.json and then folks can copy/paste that file into a region file like fr.json. Translation is happening, gonna try and get it mostly working by end of the day!

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 6, 2019

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 6, 2019

Will do!

@Walther

This comment has been minimized.

Copy link

Walther commented Apr 7, 2019

Just bumped into this app, and it looks amazing! ❤️ I would be super happy to help contribute to a Finnish translation.

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 7, 2019

AHH HURRAY!

Okay folks! The first bit of internationalization is READY!

How to contribute a new language

Create a JSON file

Copy the src/locals/en.json file to a new locale following the ISO-639-1 code. If you have a region designator and you know it, add it as an underscore as described by @briankung.

For example, if I wanted to translate to Finnish like @Walther, I would do:

mv src/locals/en.json src/locals/fi.json

Note that the convention typically uses the english name for the language, so no suomi.json! 😭

Fill out the JSON file

Go through as much as you can and translate into your language. If you can't translate everything, that's okay, it will fall back to English.

Open up src/i18n.ts and add your language

In the file, add an import line:

import fi from './locals/fi.json'

And then add it to the translations object:

i18n.translations = { fr, en };

Save your code and open up a new PR

git checkout -b finnish;
git add .;
git commit -m "Add Finnish"
git push;

Then go on github, click "New Pull Request", and then fill in your branch and master:

Screen Shot 2019-04-06 at 5 17 52 PM

Finally, give a little description of what your doing and open your PR!

Screen Shot 2019-04-06 at 5 19 37 PM

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 7, 2019

(Head's up we're still actively migrating more of the text over to the english json, so just fyi stuff might change a bit, but we'll try to keep what's in place in place)

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 7, 2019

Note that I originally suggested en_US.json as the file name, but the IETF Language Tag scheme uses dashes instead: en-US so should likely use that (unless maintainer has a strong opinion!). More info on the scheme here: https://en.wikipedia.org/wiki/IETF_language_tag

EDIT - actually kind of looks like we need the hyphenated version, if I'm looking at i18n.js correctly: https://github.com/fnando/i18n-js#using-i18njs-with-other-languages-python-php-

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 7, 2019

^ Ah good catch. 👏 I wasn't sure what that was called.

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 7, 2019

Also, this might be helpful for anyone embarking on a translation (mirror):

google sheets translation

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 7, 2019

THAT'S AMAZING!

@scaramagus

This comment has been minimized.

Copy link
Contributor

scaramagus commented Apr 7, 2019

@Flaque are there any similar plans for getquirk.app? That page could also use translations

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 7, 2019

@scaramagus that site needs some other work first, (partially just to be in a state where I can open source it) but ya I'd be down. It's just another react thing, should be the same concept. Thank you so much!!

@Walther

This comment has been minimized.

Copy link

Walther commented Apr 8, 2019

While translating, I noticed that there's still a couple of places without internationalization:

  • Settings page (history button labels etc)
  • "No thoughts yet!" icon
  • possible other places, where?

There's already a couple of translation PR's accepted. When these now-missing items will be added to the en.json, the other translations won't have values for those entries.

In what ways can we ensure the translations stay up to date? Is it possible to e.g. have some "test coverage" -like warnings / notifications for missing translation keys or such?

@devinroche

This comment has been minimized.

Copy link
Collaborator

devinroche commented Apr 8, 2019

@Walther so it will default to english if the users language is missing values. I think if you submit a pr it might be a good idea just to google translate to these languages. Then maybe if someone who is fluent catches an issue with it they can bring it up?

Thats the only solution I can think of, do you have other ideas making sure translations are up to date?

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 8, 2019

I think @Walther's suggestion isn't unreasonable - just parse the en.json file for keys that should be expected in the other translation files and at least show a warning if there are translations that are missing.

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 8, 2019

I think you're all right!

@Walther I love the idea of test coverage for translations. VS Code does something similar. We could just do a little node module; that would let us both npx run it in a github action and then also include it within the app.

With that, we could do what @briankung is saying:

just parse the en.json file for keys that should be expected in the other translation files

And we could surface that to the user, but also default to english like @devinroche said so we're not just showing nothing.

I'd definitely be down to show some sort of warning or note in the app that the version their using isn't fully translated, but my main concern is where to put it so it doesn't disrupt someone. A primary goal of Quirk is you shouldn't ever block a user who's about to enter a thought; since they're often in a rush and already in a stressful situation.

Or were you folks talking about doing it in a PR? That would be the easiest I think.

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 8, 2019

I'd love to write such a test in a PR, but I can't seem to figure out how to run the two test.ts files: src/sanitize.test.ts and src/thoughts.test.ts. I figured I'd write a new i18n.test.ts. Any guidance in getting tests running would be appreciated!

EDIT - this helped a lot, since jest was hanging on me: facebook/jest#4529

Still can't run the tests, though, I'm getting an error on the Localization import:
Screen Shot 2019-04-08 at 8 14 55 PM

@devinroche

This comment has been minimized.

Copy link
Collaborator

devinroche commented Apr 9, 2019

Yeah I think there’s an issue with expo and ts. I’ll look into this tonight! Hopefully get it fixed too.

@briankung i fixed it on my local machine after installing this @types/expo-localization. I ran into another issue though.

Screen Shot 2019-04-08 at 6 37 41 PM

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 9, 2019

I got through to the tests, but they're not requiring the spec library correctly. This is the diff I used to get there:

diff --git a/package.json b/package.json
index 63e3327..750193d 100644
--- a/package.json
+++ b/package.json
@@ -11,6 +11,7 @@
   },
   "dependencies": {
     "@expo/vector-icons": "^8.0.0",
+    "@types/expo-localization": "^1.0.1",
     "@types/react-navigation": "^3.0.1",
     "expo": "^31.0.6",
     "i18n-js": "^3.2.1",
diff --git a/tsconfig.json b/tsconfig.json
index ae96cbf..92e5a72 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -13,7 +13,8 @@
     "noUnusedLocals": true,
     "sourceMap": false, // Something else (Babel?) is creating the necessary source maps.
     "strict": false, // since we're converting from a js project
-    "target": "es2017"
+    "target": "es2017",
+    "typeRoots": [ "./node_modules/expo-localization/"]
   },
   "exclude": ["node_modules"]
 }
diff --git a/yarn.lock b/yarn.lock
index 399a276..e67ad1c 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -595,6 +595,11 @@
     react-native-safe-area-view "^0.11.0"
     react-native-screens "^1.0.0 || ^1.0.0-alpha"
 
+"@types/expo-localization@^1.0.1":
+  version "1.0.1"
+  resolved "https://registry.yarnpkg.com/@types/expo-localization/-/expo-localization-1.0.1.tgz#899c16c3f82924c286ca99f5722a6ffa4e74e84f"
+  integrity sha512-GIIFqzpI+Uu0BmKJtKGj6OwuJxSxREcpmDu1GYhbLpbZRvMdluYEmth1Q1vaIygGk1+YI8mP+G69y8VO54MSig==
+
 "@types/expo@^31.0.5":
   version "31.0.5"
   resolved "https://registry.yarnpkg.com/@types/expo/-/expo-31.0.5.tgz#54aa6ca7c97cf17fa83caedc8d0ef6ba2521003d"

Basically I added expo-localization as well as "typeRoots": [ "./node_modules/expo-localization/"] to tsconfig.json

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 9, 2019

Ex: src/sanitize.test.ts:178:1 - error TS2582: Cannot find name 'test'. Do you need to install type definitions for a test runner? Try npm i @types/jestornpm i @types/mocha and then addjestormocha` to the types field in your tsconfig.

EDIT - adding "./node_modules/@types" to typeRoots in the tsconfig.json file gets us to the next step, but more errors:

error TS2688: Cannot find type definition file for 'android'.
error TS2688: Cannot find type definition file for 'ios'.
error TS2688: Cannot find type definition file for 'src'.

@devinroche I'm going to have to turn in for the night here, but good luck!

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 9, 2019

Investigating. Git bisect says its this commit when the tests started failing: 6173108

Plan for tonight:

  • Gonna fix this
  • Gonna make sure these are running on CI so the tests don't stop passing without us realizing it

Definitely my fault, sorry bout that 😅

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 9, 2019

This is definitely happening because of the .json files, but not sure the fix yet.

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 9, 2019

Unsure if related, but even Expo itself skips the Localization module during testing: expo/expo@a3ee8cb#diff-e535def31fd598e267642218073cd353

@devinroche devinroche unpinned this issue Apr 10, 2019

@Flaque Flaque referenced this issue Apr 10, 2019

Merged

Fix tests #72

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 12, 2019

Cool, shipping this to beta on iOS for the moment so I can play with it. If you're not in the program already, have an invite link!

https://testflight.apple.com/join/F4dbdRbH

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 17, 2019

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 17, 2019

@briankung Woah. We should use that. It's got react bindings: https://github.com/projectfluent/fluent.js/tree/master/fluent-react

@briankung

This comment has been minimized.

Copy link
Contributor

briankung commented Apr 19, 2019

If you're serious, I can start work on converting our current translations to Fluent. It does look a little bit awkward in practice - I've copied one of their hello world examples to view in one page here: https://gist.github.com/briankung/3f38b5dffb71e745dc93ebd7cd9ecca6

@Flaque

This comment has been minimized.

Copy link
Owner

Flaque commented Apr 20, 2019

@briankung --> I am, I think this solves a lot of the formatting and other weird issues we've run into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.