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

Add localisation support #397

Merged
merged 32 commits into from
Nov 14, 2017
Merged

Add localisation support #397

merged 32 commits into from
Nov 14, 2017

Conversation

ph1p
Copy link
Member

@ph1p ph1p commented Nov 12, 2017

Hey folks,

I've created one type of language support and I've seen the i18next branch to late.
This is why I used another library.

I used react-intl and wrote small spartanic helper functions for main process files.
All languages are located in /locales/*.json.
There is no specific reason why I've used react-intl instead of any other i18n lib.

My main goal was to change as little code as possible and to add new languages easily.

Menu item
screen shot 2017-11-12 at 22 13 49

Some links

Issues
#285, #292, #284

Closes #285

Remove unused locale files
Add Configuration to i18n index.js
Add language menu item to view menu
Add ipc event to show restart message
Add google translator spanish
Move i18n to shared folder
Fix webpack config schema
Fix webpack extract text plugin schema
Remove babili package and add babel-minify-webpack-plugin (babel/minify#124)
@sallar
Copy link
Member

sallar commented Nov 13, 2017

Thanks @ph1p for the great work. I quite like this. It's ok to use react-intl as far as I can see, since obviously its easy to use the translations in non-react files too.
Before I review the code, could you check why are the tests failing?

@ph1p
Copy link
Member Author

ph1p commented Nov 13, 2017

@sallar Thank you! I have a problem with the current version of buttercup. I can't run npm run build:renderer. This is why I updated all outdated packages (That did not help).
Webpack freezes at 91% additional asset processing and node throws FATAL ERROR: CALL_AND_RETRY_LAST.
Your last working commit was 8732150474e9bf6be7cb78faf9bff7f17d9049e2.
And the problem is babili (babel-minify). I replaced it with uglify and commented out babel-minify. Now it works, hopefully :)

Update i18n config and call correct formatMessage method
Remove contenthash from exported css file
Add minimize css option
Copy link
Member

@sallar sallar left a comment

Choose a reason for hiding this comment

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

Thanks @ph1p. This looks great. I requested some changes that are small... I would also need to run the app to see how it looks after you do the changes :) Cheers!

@@ -47,7 +70,55 @@ module.exports = merge(baseConfig, {
new webpack.DefinePlugin({
'process.env.NODE_ENV': JSON.stringify('production')
}),
new BabiliPlugin(),
new UglifyJSPlugin({
Copy link
Member

Choose a reason for hiding this comment

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

Does uglifyJS support ES6 which we are using? I think the reason I used Babili was that it was the only one that supported ES6 out of the box.

Copy link
Member Author

Choose a reason for hiding this comment

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

Jep. Documentation says, that i can use the ecma option (https://github.com/webpack-contrib/uglifyjs-webpack-plugin#uglifyoptions)

}
}
}),
// new MinifyPlugin({
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this unused comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

package.json Outdated
@@ -108,36 +108,37 @@
},
"homepage": "https://buttercup.pw",
"dependencies": {
"opencollective": "^1.0.3"
"opencollective": "^1.0.3",
"react-intl": "^2.4.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to devDependencies? Since it will be compiled to the final package

Copy link
Member Author

Choose a reason for hiding this comment

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

Jep

import pkg from '../../package.json';

const defaultTemplate = [
Copy link
Member

Choose a reason for hiding this comment

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

Is this diff block because you moved this constant to the setup function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because I need the store variable for all menu points.

@@ -61,10 +63,17 @@ ipc.on('will-quit', () => {
store.dispatch(setUIState('isExiting', true));
});

// show message, when locale changed
ipc.on('locale-changed', (e, payload) => {
alert(payload);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the showDialog function in renderer/system/dialog.js?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why does the user need to relaunch the program? wont react-intl change the texts in place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Jep

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I can't find a solution to update the whole electron menu.

Copy link
Member

Choose a reason for hiding this comment

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

@ph1p We do that when a new archive is added. For example I call it everytime the store gets updated: store.subscribe(debounce(() => setupMenu(store), 500));. So this should also update the language already if everything's setup correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok cool. I'll try it :)

Copy link
Member Author

@ph1p ph1p Nov 14, 2017

Choose a reason for hiding this comment

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

Yeah, it works!

const translations = [];

config.availableLanguages.forEach(lang => {
translations[lang.code] = require(`../../../locales/${lang.code}`);
Copy link
Member

Choose a reason for hiding this comment

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

I think this wont work when compiled. Since the paths will change. in menu.js I do this for resolving: path.resolve(__dirname, `../resources/icons/${item.icon}.png`) and I think this should be similar.

Copy link
Member

Choose a reason for hiding this comment

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

Also can we use .map here?

const translations = config.availableLanguages.map(lang => require(...));

Copy link
Member Author

Choose a reason for hiding this comment

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

It will compile. I've made the forEach, because if you dynamically require content, webpack typically ignores it. With this little trick I require all translations and load them into the object. So when I add a language to the config object, the translations are automatically added.
Anyways, I have updated my code and decided to import the languages by hand.

locales/de.json Outdated
"system.dialog.confirm": "Bestätigen",
"system.dialog.nevermind": "Abbrechen",
"system.dialog.password": "Passwort",
"main.menu.language": "Sprache",
Copy link
Member

Choose a reason for hiding this comment

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

I really like the nested style, though I think some components should be named differently. For instance, main.menu alludes to there being a main category with a menu subcategory, which I don't think there is. Should this be main-menu perhaps? Or even menu.main?

locales/en.json Outdated
"system.dialog.confirm": "Confirm",
"system.dialog.nevermind": "Nevermind",
"system.dialog.password": "Password",
"main.menu.language": "Language",
Copy link
Member

Choose a reason for hiding this comment

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

And here as well..

locales/es.json Outdated
"system.dialog.confirm": "Confirmar",
"system.dialog.nevermind": "No importa",
"system.dialog.password": "Contraseña",
"main.menu.language": "Idioma",
Copy link
Member

Choose a reason for hiding this comment

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

And here..

@perry-mitchell
Copy link
Member

Also, this shouldn't be merged until we've had a review of the Spanish and German content 😊

@ph1p
Copy link
Member Author

ph1p commented Nov 13, 2017

I'm german, so I reviewed the german part by myself ^^.
I only added spanish language, because of ticket #284.
A girlfriend of mine speaks spanish, she will translate it.
These language files are only examples. I'll write a little explanation about the naming and update all of them.


Thank you for reviewing my code. I give my best to solve all problems.

…se.js -> resolve -> alias)

Add full reload after language change without restarting the app
Remove  babili (babel-minify)
Move react-intl to dev dependencies
@ph1p
Copy link
Member Author

ph1p commented Nov 13, 2017

I've updated my code and have decided to hold the naming of language ids easier.
The id describes or is itself the translated word written in lower kebab-case.
I think it's better, because it's shorter and easier to use with duplicate words.

@perry-mitchell
Copy link
Member

perry-mitchell commented Nov 14, 2017

I can't find the word Trash

Ah.. well this is more complex. General and Trash are groups - content within the Archive data. These are created as boilerplate items in buttercup-core. I agree that these should be translated - I can add an option to the core for this, but it should be done later imo (not part of this PR).

Task in core: buttercup/buttercup-core#191

@ph1p
Copy link
Member Author

ph1p commented Nov 14, 2017

@perry-mitchell Ah ok and take look at password generator, this must also be dynamic.

@perry-mitchell
Copy link
Member

password generator, this must also be dynamic

Agreed.. I think we need to figure out a nice way to pass translations around. I guess the generator, being a separate component, should have it's own internationalisation, but should take the current language from the host application (ie. the desktop application in this case).

@perry-mitchell
Copy link
Member

perry-mitchell commented Nov 14, 2017

Hmm.. appveyor is still dying. Issue seems to be with npm. @ph1p would you mind trying to add npm cache verify to the appveyor build script? Before npm install?

If that doesn't work, npm cache clean before npm install might fix it.

EDIT: Just updated it.. let's see how it goes..

@ph1p
Copy link
Member Author

ph1p commented Nov 14, 2017

Ok guys ... I've tried several things, but I don't know why travis and veyor failing. There're stopping at ava && npm run lint. I've tested it on my own and can't reproduce it.

@sallar
Copy link
Member

sallar commented Nov 14, 2017

@perry-mitchell @ph1p I'll have a crack at the app veyor thing to see whats dying.

@sallar
Copy link
Member

sallar commented Nov 14, 2017

@ph1p Im going to do some commits to your repo if you dont mind. including reverting some of the package.json upgrades. It's too many changes for this PR and maybe its one of them that breaks the whole thing.

@sallar
Copy link
Member

sallar commented Nov 14, 2017

@perry-mitchell @ph1p done!

@ph1p
Copy link
Member Author

ph1p commented Nov 14, 2017

Thanks @sallar. This was a stupid idea of mine, to push all code into the master branch 🤗. Next time I’ll create an extra feature branch and another pull request.

@perry-mitchell
Copy link
Member

This was a stupid idea of mine

No such thing! This was a huge chunk of work. We obviously need to update packages anyway.. 😁

@ph1p
Copy link
Member Author

ph1p commented Nov 14, 2017

Ok I've one last commit with some bugfixes and translations (no dependency updates). In about 20 minutes I'll push it.

Fix context menu translations
Translate errors
@sallar
Copy link
Member

sallar commented Nov 14, 2017

Great work @ph1p. I'm going to merge this :)

@sallar sallar merged commit bb9ee4f into buttercup:master Nov 14, 2017
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.

3 participants