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

Added internationalization (german included atm) #184

Closed
wants to merge 3 commits into from
Closed

Added internationalization (german included atm) #184

wants to merge 3 commits into from

Conversation

DarkSmile92
Copy link
Contributor

Done with react-localize-redux.
comments with "// TRANSL:" means these lines must be customized to add more languages

  • ErrorBoundary not translated
  • Form on start has an inactive Tab at the top which does not get translated when changing language. Only after opening.
  • Middlewares not translated
    Manta\app\helpers\form.js -> Messages not translated, did not find a nice way without passing too many arguments. Maybe you can find one?
  • Messages from Middlewares not translated
    ex.: Manta\app\middlewares\FormMW.jsx -> I think here is a good place to insert translation handler and pass to validateFormData()
  • Menu not translated

Working on the rest of it.
Maybe you have some ideas on how to translate the middlewares and the form methods in a good way :)

@hql287
Copy link
Owner

hql287 commented Jan 21, 2018

Robin @DarkSmile92 , sorry for taking a bit long to get back to you.
Before we go further, just wanted to know why you decided to choose react-localize-redux over other solutions such as react-i18next or the battle-tested react-intl?

@hql287
Copy link
Owner

hql287 commented Jan 21, 2018

Also, this poses another question: How should developers and translators work together?

Meaning how can we allow more than 1 translators to work on each language? So they can cross-check each other to make sure the translation is correct?

@@ -85,6 +91,18 @@ class App extends PureComponent {
dispatch(UIActions.changeActiveTab(tabName));
}

changeLang(locale) {
const { dispatch } = this.props;
if (this.props.currentLanguage == 'en') {
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be the other way around? :)

@SkyzohKey
Copy link

React-intl should be used instead because it uses ICU message format and thus allows for crowd translating. :/

Also, coupling the locale to the redux store looks like a bad idea to me in term of performances

@DarkSmile92
Copy link
Contributor Author

I think your React knowledge is way more advanced than mine so I can't really tell how this would have an impact on the performance @SkyzohKey . I will trust your judgement.

@hql287 : react-localize-redux seemed the simpliest way to integrate in the existing components without too much changes for me. As said before I am an experienced developer, but not so much with React. I checked the other two mentioned by you and now I think react-intl would be better (also based on the statement from @SkyzohKey ). So if you agree, I will revert the changes and implement them with React-intl during this week :-)

@hql287
Copy link
Owner

hql287 commented Jan 22, 2018

So if you agree, I will revert the changes and implement them with React-intl during this week :-)

@DarkSmile92: Thanks! 👍

Btw, I also just had it working with react-i18next. I haven't gone through all the features of react-i18next yet but I think it's much simpler than react-intl, considering these:

  • No webpack/babel configuration needed
  • Lazy load translations using namespace
  • react-18next has its own store so there's no need to worry about redux.

It seems to me that the only thing that you need to do is to add the t function to your components then you're good to go. To be honest, I had a much better experience than what I had with react-intl and I'm actually quite happy with the way this turned out.

That said, I'll play with it some more to see if there's is any limitation. Will open the PR up later today so everyone can see.

In the meantime, you're very welcome to try react-intl, then maybe we can do a side-by-side comparison to see which one would be better for Manta. Let me know what you think.

Also, coupling the locale to the redux store looks like a bad idea to me in term of performances

@SkyzohKey: Agree! Have you tried react-i18next yet? Would love to hear your thought about its pros and cons in comparison with react-intl. Thanks! 👍

@DarkSmile92
Copy link
Contributor Author

@hql287 Sure let's do it that way and compare it. The decision is up to you in the end :) So I am happy to help out and curious which one will fit better to manta. I'll do my best to have it asap.

@hql287
Copy link
Owner

hql287 commented Jan 22, 2018

@DarkSmile92 Great. Thanks!

@hql287
Copy link
Owner

hql287 commented Jan 22, 2018

@DarkSmile92: Just created the PR #187, feel free to take a look and let me know what you think.

@DarkSmile92
Copy link
Contributor Author

@hql287 I just tried to implement react-intl last night. During a break I reviewed your PR with react-i18next and for my opinion react-intl is a little bit of overkill.
First of all with react-i18next we just need to define a new component, import it in every component we want to use translation and then just call the t-function.

With react-intl this is not that simple (at least more lines of code) as we have to use an other component called FormattedMessage which breaks the flow and does not integrate so well with the existing code in my opinion.
Also we need to wrap the whole App in an IntlProvider.

<FormattedMessage
                        id="form.settings.general"
                        defaultMessage="General"
                    />

I don't see an advantage in using react-intl over react-i18next. I would go with react-i18next because it integrated much smoother into the existing code and does not blow up the components that much.

Give me your thoughts on it!
Would be an honour the take translation further so you can focus on other points :)

@hql287
Copy link
Owner

hql287 commented Jan 23, 2018

a little bit of overkill.

@DarkSmile92 This is exactly how I felt when I tried to integrate react-intl the first time. And I agree with everything that you mentioned, such as using FormattedMessage vs t or breaking the workflow.

Anyway, looks like our comparison is already in place. Feel free to keep playing with react-intl or close the PR.

Would be an honour the take translation further so you can focus on other points :)

👍 I'm setting up Crowdin for Manta, will let you know when it's ready. Thanks.

@DarkSmile92
Copy link
Contributor Author

@hql287 I would rather help implementing react-i18next so I will close this PR.
Can I contribute anything besides the german translation texts? I can take your PR and work on the react-i18next integration

@hql287
Copy link
Owner

hql287 commented Jan 23, 2018

@DarkSmile92: I just update #187 with detailed instruction on translating Manta using Crowdin. Feel free to transfer your translation from the PR to Crowdin when you're ready.

Can I contribute anything besides the German translation texts? I can take your PR and work on the react-i18next integration

If you look at the TODOs in my PR, you'll see there's not much left to do actually. I think it would be easier for me to finish it and better for you to work on a new issue :)

May I suggest this #178 and #177? They seem pretty interesting to me.

Or, maybe you can work on some specific feature that would be helpful for German users? (I remember you mentioned some of them in our previous emails.)

@DarkSmile92
Copy link
Contributor Author

@hql287 I will have a look at #178 and #177 . Also I will see through the german requirements and create issues accordingly (with information that I will work on it). Or is there another way you want to manage the tasks?

@hql287
Copy link
Owner

hql287 commented Jan 23, 2018

Also I will see through the german requirements and create issues accordingly (with information that I will work on it). Or is there another way you want to manage the tasks?

@DarkSmile92 You actually can create issues, comment and even host discussions with other translators directly on Crowdin.

screen shot 2018-01-23 at 21 56 43

@SkyzohKey
Copy link

SkyzohKey commented Jan 23, 2018

@DarkSmile92 @hql287 I recommended react-intl because it has a very powerful system for managing localized currencies, dates, strings, prices, etc and as Manta is dealing with all these stuff, that would have allowed to get a more robust system. Also, i has no experience with react-i18next so I can't say bad/good stuff about it. :)

@DarkSmile92 About handling translation in a redux store, it cause performances bottlenecks because of how redux works. Even if you use 1 translation in a component, the store will still load ALL the translations in memory because of how a reducer/state works. + Once you change the locale at runtime, components will be queued for re-render from top-to-bottom and potentialy freeze the app on slower devices. ;)

Anyway, I'm happy you found what suits best Manta and will happily translate it to french once I get time ;)

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.

3 participants