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

fix(localization): various localization improvements #193

Merged
merged 17 commits into from Aug 14, 2017

Conversation

lex111
Copy link
Member

@lex111 lex111 commented Aug 2, 2017

  • Add searchType to localization
  • Localization updateText when change language + minor fixes
  • Update title on options screen when change language
  • Add {{payload}} (action and ref_type,) to localization file for events screen (eg, "created", "edited", "moved" or "deleted", "commented", etc). See details here. (Important)
  • Add support for localization for moment (done, but needs review!)
  • Add dynamic language output (view code). (i.e. so that we do not add it manually each time when a new language is supported).
    Possible solution: export all available languages (locale/languages/index.js). A new key is added: originalName (or simply title) - this is the national name of the language (it will be displayed). Or something like this (information about the language at the beginning of each language file):
  • Localize Members and Description on organization screen
  • Localize Stars, Forks, forked from, View All (PR list) on repository screen
  • Localize Open, Close on PR screen
  • Localize Post (comment input component)
  • Localize title for PR's and Issues

Do not hesitate to update this PR, if you want!

Copy link
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

👍

@andrewda andrewda changed the title Localization fixes fix(localization): various localization improvements Aug 11, 2017
@lex111
Copy link
Member Author

lex111 commented Aug 12, 2017

I did dynamic output of languages (from file language-settings.js), look, please.

Also I suggest to show native names of languages, instead of in English, i.e. Deutsch instead of German, for example, how do you like this proposal?

@lex111 lex111 mentioned this pull request Aug 12, 2017
y: '%dy',
yy: '%dy',
},
const language = I18n.locale.substr(0, 2);
Copy link
Member Author

Choose a reason for hiding this comment

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

I have done so for the time being, but this is essentially the initialization of the moment, and it should not be executed here (where can it be moved?), it first. And secondly, only the device language is taken into account here, but if the language was changed in the settings, then the date/time language (ie moment) will remain unchanged (we must also take into account the language in the storage). What can we do about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I solved this problem, you can see in this commit. Now the configuration of the locale is made in App.js, as it probably should be there and localization works as I described it in my comment.

In my opinion, it turned out pretty good, as usual, I'm waiting for your feedback 🙌

@lex111
Copy link
Member Author

lex111 commented Aug 12, 2017

Update title on options screen when change language

I need help! How can we re-render the title when changing the language?
image

@lex111
Copy link
Member Author

lex111 commented Aug 12, 2017

Also, look at this commit, I did support the localization of events better, it looks like it worked out well, but I want to know your opinion 😌

@Antoine38660
Copy link
Member

Antoine38660 commented Aug 12, 2017

@lex111 you said:

Update title on options screen when change language

I need help! How can we re-render the title when changing the language?
image

Two "solutions":

@lex111
Copy link
Member Author

lex111 commented Aug 12, 2017

@Antoine38660 thanks, but unfortunately did not work NavigationActions.SetParams for me, the title update did not come through.

@lex111
Copy link
Member Author

lex111 commented Aug 12, 2017

@Antoine38660 hurraaay, I did it, I misunderstood how to use setParams, in general, you can shoot a smiley of disappointment, it's works! 😄

@Antoine38660
Copy link
Member

Awesome dude! 🎉 🎉 🎉

11/11 checks. I'll look at your code tonight 😃

Copy link
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

🔥

Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

This is such an amazing PR 😍 😍 😍

I can't see anything that needs changing from the onset. Thank you so much @lex111 we really owe you one <3

@housseindjirdeh housseindjirdeh merged commit 9c3e182 into master Aug 14, 2017
@housseindjirdeh
Copy link
Member

Woops I'm sorry if I merged it too prematurely in case you wanted to take a look as well @Antoine38660. Please let us know if you see anything that should be changed and we can address it in a future PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants