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

Internationalization (i18n) with react-i18next #921

Merged
merged 11 commits into from Dec 13, 2018
Merged

Internationalization (i18n) with react-i18next #921

merged 11 commits into from Dec 13, 2018

Conversation

mrkvon
Copy link
Contributor

@mrkvon mrkvon commented Dec 6, 2018

Proposed Changes

  • Testing an react-i18next library. So far works fine.

Largely following tutorials on https://react.i18next.com.

Testing Instructions

Fixes #492

TODO:

  • hide behind a feature flag
  • test translation extraction tools and translating tools

@mrkvon mrkvon changed the title Internationalization (i18n) with react-i18next [WIP] Internationalization (i18n) with react-i18next Dec 6, 2018
@mrkvon mrkvon changed the base branch from master to react December 6, 2018 23:59
@guaka guaka added the i18n label Dec 7, 2018
@mrkvon mrkvon changed the base branch from react to master December 7, 2018 16:26
@mrkvon mrkvon changed the base branch from master to react December 7, 2018 19:04
@mrkvon mrkvon changed the base branch from react to master December 7, 2018 19:04
import i18n from 'i18next';
import { reactI18nextModule } from 'react-i18next';
import * as translationEN from '../../../../public/locales/en/translation';
import * as translationCS from '../../../../public/locales/cs/translation';
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably better off starting with some ISO standard codes; perhaps the same as we use for profile languages? (ISO 639 2)

Copy link
Contributor Author

@mrkvon mrkvon Dec 10, 2018

Choose a reason for hiding this comment

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

Ok, let's use the ISO 639-2 instead of 639-1. It includes more languages and we already use it as @simison describe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh bummer, I think I've looked up something wrong here.

I picked the ISO standard from this sentence:

The languages.json is a custom made file which contains languages from languages_orig.json that has iso_639_2 standard defined.

But it just states that we pick only those languages from the whole massive list and then actually in the database we use codes like fi_FI:

https://github.com/Trustroots/trustroots/blob/15a8bcebee8286eca1591f950a120420b2e08dd3/modules/core/client/directives/tr-languages.client.directive.js

Copy link
Contributor Author

@mrkvon mrkvon Dec 11, 2018

Choose a reason for hiding this comment

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

Would you like to revert to ISO_639-1, then? It should be as easy as dropping a commit.

Actually, ISO_639-1 will be compatible with moment.locale(), i18next documentation etc.
And when we need to use languages out of the scope of ISO_639-1, we can use ISO_639-3 just for them.

I would prefer not to use fi_FI format though - not a standard, redundant information and hard to reuse in localization library configs without custom parsing.

Copy link
Contributor

@simison simison Dec 11, 2018

Choose a reason for hiding this comment

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

Sounds reasonable 👍 I'll leave it up to you to decide since you have better insight. :-) We can very easily refactor at any point if we hit issues. That said, probably easier to get it right from the beginning.

Do ISO_639-1 or ISO_639-2 make difference between localized languages, such as English GB/US or French CA/FR etc? In volunteering space those are often pretty fast to crop up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I made some false assumptions (i.e. fi_FI is not a redundant information). I'll research more.

@simison
Copy link
Contributor

simison commented Dec 7, 2018

What do you think if we instead use withNamespaces higher order component like so:

const Volunteering = () => {
 return <Something />;
}

export default withNamespaces()(Volunteering);

That way if function/component is returning multiple times, we wouldn't need to wrap each return with NamespacesConsumer:

export default const Volunteering = () => {
  if (something) {
    return (
      <NamespacesConsumer>
        <Something />
      </NamespacesConsumer>
    );
  } 

  return (
    <NamespacesConsumer>
      <Something />
    </NamespacesConsumer>
  );
}

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 9, 2018

use withNamespaces higher order component

It's nice, yes. There is just a check in the code for importing react components to angular, which is failing with this. It could be possible to drop that check, too.

Update:
I'm facing a row of errors when I try to use withNamespaces. It's related to importing the React component to Angular.
Since the alternative solution just works, I'd prefer to focus on other things at the moment. Eventually we can attempt to change it again.

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 10, 2018

It's very important that we figure out what localization tool we can use. (A tool that non-programmers can use to translate the files in /public/locales/**/translation.json.) It would preferably be free and open source and run on different operating systems.

It makes a difference between choosing this library and not.

Thank you @Akronix for pointing this out to me.

@mrkvon mrkvon force-pushed the react-i18next branch 2 times, most recently from 79e1a29 to bd129a9 Compare December 10, 2018 14:31
@simison
Copy link
Contributor

simison commented Dec 10, 2018

It's very important that we figure out what localization tool we can use.

Good point! I don't see us being limited to anything specific, because i18n-next has utils for to/from Gettext and other formats as well even a Gettext Webpack loader:

https://www.i18next.com/overview/plugins-and-utils#utils

Gettext is about as free and open as you can get so we can either use self-hosted software, SAAS software to even local tools such as Poedit.

i18n-next seems great also because it has support for such a variety of frameworks, among others Express.js (i18next-express-middleware) which we use at the backend so we can translate our emails and other backend served stuff.

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 10, 2018

Seems react-i18next could be the library, then.

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 10, 2018

A design of language switch button would be nice.
How should it behave, where should it be placed?

@simison
Copy link
Contributor

simison commented Dec 11, 2018

A design of language switch button would be nice.
How should it behave, where should it be placed?

For non-authenticated users, it could always be visible in the header. We can get quickly going with a simple Bootstrap navigation dropdown:

image

It should fit also in mobile header nicely:

image

(and if it doesn't, using a shortening "FI" for Finnish etc should work.

We'll hit some limits with this when we add more languages but we can work more on it then. I have some old designs from BeWelcome hackathon/early Trustroots times that should be useful.

Technically I like the idea of using a <select> here, but I don't think it works nicely styles-wise so it would probably be just more work to make it work nicely. Dropdown works straight out of the box.

For authenticated users, we don't need to show it all the time (as they really need to change it just once). Instead, we can add it to their "account" page as <select>:

image

This is the language of the interface you see across the site.

Thanks to all our community members who helped translate! You can help us out!

Probably good to hide these selections behind a feature flag until we have something to show?

Thoughts?

@mrkvon What were you thinking specifically when asking "How should it behave"?

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 11, 2018

What were you thinking specifically when asking "How should it behave"?

@simison You answered precisely what I was asking. Thank you!

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 11, 2018

We can save the preferred language in a client (set a cookie) or in a database (PUT /api/users, { "localization": "en_GB", ... }). Which one?
Probably a separate PR.

@simison
Copy link
Contributor

simison commented Dec 11, 2018

We can save the preferred language in a client (set a cookie) or in a database (PUT /api/users, { "localization": "en_GB", ... })

Cookie sounds good to me as a starting point since that's what unregistered users need.

For authenticated users, we can store it in their user table just like any other account content.

Probably a separate PR.

Totally 👍

@mrkvon mrkvon changed the title [WIP] Internationalization (i18n) with react-i18next Internationalization (i18n) with react-i18next Dec 12, 2018
@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 12, 2018

Wants to be merged. Somebody should succeed with using the tools for extracting translations, and tools for translating. I'll try tomorrow.
We can continue with this only when we know both programmers and non-programmers can create translations frustration free.

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 12, 2018

@guaka
Exctracting translations - react-i18next documentation

Also we should decide whether we want to use natural, or keybased catalog. Currently we use natural one (translation keys equal English values).

The i18next-gettext-converter would be nice to have plugged to make many translation tools available. And other plugins.

@simison
Copy link
Contributor

simison commented Dec 12, 2018

we should decide whether we want to use natural, or keybased catalog. Currently we use natural ones (translation keys equal English values).

I'd go for natural like it's now — it removes some mental complexity while developing and moves the overhead of "changing strings" to translation work instead. I think that's fine since we aren't changing strings that often.

Wants to be merged

Code is looking good 👍 just need to give this a spin (aiming to do that tonight).

@simison
Copy link
Contributor

simison commented Dec 12, 2018

This seems to be a bit broken now on development version:

image

Let's put the flag off by default for all environments and folks working on this can enable locally?

Copy link
Contributor

@simison simison left a comment

Choose a reason for hiding this comment

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

Looks great and tests well!

Let's just hide the <select> in the header for now from all environments (just via feature flag) before merging so that master won't feel broken even in the development environment.

There will be quite a few tasks as a follow-up but we can work on this incrementally.

Nice job handling this; 🚢 it!

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 13, 2018

Opened an issue for i18next-parser: i18next/i18next-parser#145

@mrkvon
Copy link
Contributor Author

mrkvon commented Dec 13, 2018

Ok, let's make a leap of trust that the localization tools will work.
Merging.

@mrkvon mrkvon merged commit 404b862 into master Dec 13, 2018
@simison
Copy link
Contributor

simison commented Dec 13, 2018

As long as things are behind a feature flag and not breaking anything or making the build enormous, we're safe to iterate and refactor. 👍

@guaka
Copy link
Contributor

guaka commented Dec 14, 2018

What about already enabling the language switcher for users with the "beta" role?

@simison simison deleted the react-i18next branch December 16, 2018 18:53
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.

None yet

3 participants