Create combined translation keys file #170
Conversation
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
looks great! no comments from me 💃
package.json
Outdated
| }, | ||
| "scripts": { | ||
| "combine-translation-strings": "babel-node utils/combineTranslationKeys.js", | ||
| "find-strings": "babel-node utils/findLocaleStrings.js", |
There was a problem hiding this comment.
actually find-strings could be changed to npm run make:translations
There was a problem hiding this comment.
or better
npm run make:locale or npm run make:localization
There was a problem hiding this comment.
we're not making translations or localizations, we're making the list of strings from which someone would make the required translations
There was a problem hiding this comment.
or whatever. make:something ;)
There was a problem hiding this comment.
make:translation-strings
package.json
Outdated
| "url": "https://github.com/plotly/react-plotly.js-editor/issues" | ||
| }, | ||
| "scripts": { | ||
| "combine-translation-strings": "babel-node utils/combineTranslationKeys.js", |
There was a problem hiding this comment.
npm run make:combined-locales ?
Just so it fits in with all the other make stuff?
|
Since this 'makes' another build artifact required to run in prepublish you could give it a 💃 other than that! |
|
haha this approval button's pretty strict, another 👀 @bpostlethwaite ? |
utils/combinedLocales.js
Outdated
| const pathToCombinedTranslationKeys = path.join( | ||
| __dirname, | ||
| 'combinedTranslationKeys.txt' | ||
| 'combinedLocales.txt' |
There was a problem hiding this comment.
Sorry to nitpick but as @alexcjohnson suggested perhaps we should continue calling these translation-references or translation-strings or something. Locales are the full objects with all the fancy dateFormats and such right?
There was a problem hiding this comment.
plotly.js calls it's file translation-keys, I could call it translation-keys for the editor too, and combined-translation-keys for the combined file, works?
There was a problem hiding this comment.
plotly.js has a rule of all-lowercase filenames, which seems to have fallen out of fashion in more recent js projects, but I won't vote against consistency across repos, I like your proposed names @VeraZab.
|
ok, how about those filenames? and also changed some scripts around |
@bpostlethwaite @alexcjohnson for your review