-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
i18n + FR #1610
i18n + FR #1610
Conversation
I've added a few |
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 add customIDs to make translations easier and more consistent. Also please include the package-lock.json
, this file should be committed according to npm docs. Otherwise this looks good from my side.
Oh and regarding the added |
package-lock.json is included, only yarn.lock was removed because that's not the package manager you mention to use. |
Ah missed that, just saw that it deleted like 20k lines from package.lock.json, so I thought you have deleted the whole file, you can disregard my comment then of course |
src/app/config/setup/octoprint-authentication/octoprint-authentication.component.ts
Show resolved
Hide resolved
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.
Just some really minor changes, looks great otherwise. Thank you! I don't know any french, so I can't comment on the translation file.
I have 2 questions:
updateLocales
method. If i got everything right this will only be used on a dev machine, right? Maybe it would make sense to move this to separate file then, so we don't need to ship it with that function. Maybe you could also explain what are the benefits of this function, does it add new translation items to already existing translations?- This doesn't support live updating yet, right? My idea would be to have this configurable in the settings in a later version. Did you read anything on whether that is possible?
helper/locale.js
Outdated
if (err) throw new Error(err.message); | ||
|
||
// hard copy of messages.xlf | ||
const newTranslation = JSON.parse(JSON.stringify(extracted)); |
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.
The project also has lodash installed, maybe use deepCopy from lodash, might be a bit faster: https://lodash.com/docs/4.17.15#cloneDeep
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.
will do
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.
using const _ = require('lodash-es');
i get
internal/modules/cjs/loader.js:1015
throw new ERR_REQUIRE_ESM(filename, parentPath, packageJsonPath);
^
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /home/peac/Documents/vsp/octodash/node_modules/lodash-es/lodash.js
require() of ES modules is not supported.
require() of /home/peac/Documents/vsp/octodash/node_modules/lodash-es/lodash.js from /home/peac/Documents/vsp/octodash/helper/locale.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename lodash.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /home/peac/Documents/vsp/octodash/node_modules/lodash-es/package.json.
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1015:13)
at Module.load (internal/modules/cjs/loader.js:863:32)
at Function.Module._load (internal/modules/cjs/loader.js:708:14)
at Module.require (internal/modules/cjs/loader.js:887:19)
at require (internal/modules/cjs/helpers.js:74:18)
at Object.<anonymous> (/home/peac/Documents/vsp/octodash/helper/locale.js:1:11)
at Module._compile (internal/modules/cjs/loader.js:999:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
at Module.load (internal/modules/cjs/loader.js:863:32)
at Function.Module._load (internal/modules/cjs/loader.js:708:14) {
code: 'ERR_REQUIRE_ESM'
}
using import _ from 'lodash-es';
i get
/home/peac/Documents/vsp/octodash/helper/locale.js:1
import _ from 'lodash-es';
^^^^^^
SyntaxError: Cannot use import statement outside a module
at wrapSafe (internal/modules/cjs/loader.js:915:16)
at Module._compile (internal/modules/cjs/loader.js:963:27)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
at Module.load (internal/modules/cjs/loader.js:863:32)
at Function.Module._load (internal/modules/cjs/loader.js:708:14)
at Module.require (internal/modules/cjs/loader.js:887:19)
at require (internal/modules/cjs/helpers.js:74:18)
at [eval]:1:1
at Script.runInThisContext (vm.js:120:18)
at Object.runInThisContext (vm.js:309:38)
Is it worth investigating further and probably packaging a new module ?
src/app/config/setup/octoprint-authentication/octoprint-authentication.component.ts
Show resolved
Hide resolved
The function will check IDs in the html/ts files and update (add/remove) entries in all translation files accordingly. I wrote it because i was tired of copy/pasting 200+ translations manually to french at each update, and scared imagining all the work it would require to update all languages on any change.
Not yet but it can be easily implemented, it's just a matter of changing the url to /lang/index.html. |
Ah ok that update script thing is a great addition then! Feel free to just leave it as is then. I'm planning to redesign the settings menu (to make more use of the available screen estate and simplify a few things) in v3.1 or v3.2, I'll probably come back to this then. |
Great ! will it be architectured with flex ? It's going off-screen on the bottom on our side with NOX theme, and was also a little bit short on space without a theme |
Probably going to be flex layout. |
Ok pipeline is up and running again. Please rebase or merge main. Once the last three comments have been addressed this is ready to be merged :) |
ready to merge once you re-generate package-lock |
Ok looks good now. I'll update the package-lock tomorrow and check whether I manage to import lodash in the backend and then merge after that :) |
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.
🚢
* add localization tags and 3 translation examples * build all locales, warn on missing translation * Version française V1.0 de la traduction * delete yarn.lock * update package.json with upstream * re-add @angular/localize * fix merge * re-add extract command * add ids * replace added spans by ng-containers * fix duplicate id * replace spans by ng-containers * basic docs * Prise en compte des review pour la traduction française V1.00 * remove irrelevant change * cleanup angular.json * fix FR translations and merge ids * revert to fr instead of fr-FR * revert fr-FR to fr * fix targets that were sources * revert out-file * revert package-lock * install angular i18n * fix build issues * enable serving custom locales, build detects host lang * update command name * correct source language * update locales * add target language * add missing semicolons * fix duplicate url setting * fix sourcelocale basehref * fix typo * Nettoyage de coquilles de traduction fance V1.00 * remove irrelevant files * Nettoyage de doublons de traduction France V1.00 * fix adjust screen * fix spacing * comment fixes * translation adjustments * please linter * please linter * trailing whitespace * lint autofix * use lodash and update package-lock.json * revert lodash Co-authored-by: EtherIoo <thierry.dubosse@eterioo.com> Co-authored-by: UnchartedBull <unchartedbull@gmail.com>
npm run locale:extract
npm run locale:update
fr
translationLANG
)TODO
remove yarn.lock from PRcorrect fr translationmake sure it builds correctlyadd localization documentationreplace added spans by ng-containersset customIDsFixes #914