-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Centralized languages into 1 files #1818
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
Conversation
sosukesuzuki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please confirm my reviews!
| <option value='ru'>{i18n.__('Russian')}</option> | ||
| <option value='es-ES'>{i18n.__('Spanish')}</option> | ||
| { | ||
| languages.map((language) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
() for a single argument and return are unnecessary in arrow functions.
please rewrite like below:
languages.map(language =>
<option value={language.locale} key={language.locale}>{i18n.__(language.name)}</option>)
browser/lib/i18n.js
Outdated
| const { app } = remote | ||
| const { languages } = require('./Languages.js') | ||
|
|
||
| let locales = languages.reduce(function (localeList, locale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
locales is never reassigned. Please use const instead of let.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this duplicates with this lines.
Please export locales from this file and import locales from this file in browser/main/Main.js.
browser/main/Main.js
Outdated
| 'ru', | ||
| 'es-ES' | ||
| ] | ||
| let locales = languages.reduce(function (localeList, locale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please confirm this review
|
@sosukesuzuki please confirm my changes |
|
Good! I'll approve!!:+1: |
kazup01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Merged. Thank you for amazing contribution @ZeroX-DG ! |
This PR centralized all languages into 1 file so that next time you want to add a new language just add it to the Languages.js file.