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

i18n + FR #1610

Merged
merged 51 commits into from
Apr 11, 2021
Merged

i18n + FR #1610

merged 51 commits into from
Apr 11, 2021

Conversation

pciavald
Copy link
Contributor

@pciavald pciavald commented Mar 25, 2021

  • add package @angular/localize
  • add command npm run locale:extract
  • add command to update translation files npm run locale:update
  • add setting to generate locales in serve and build modes
  • add i18n and $localize markers to all locale strings with custom IDs
  • add fr translation
  • use host's language by default (env LANG)
  • update readme.md and contriburing.md

TODO

  • remove yarn.lock from PR
  • correct fr translation
  • make sure it builds correctly
  • add localization documentation
  • replace added spans by ng-containers
  • set customIDs

Fixes #914

@pciavald
Copy link
Contributor Author

I've added a few <span>s to help putting the tags, that needs double-checking

Copy link
Owner

@UnchartedBull UnchartedBull left a 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.

src/app/app.component.html Outdated Show resolved Hide resolved
@UnchartedBull
Copy link
Owner

Oh and regarding the added spans, please replace them with ng-container. ng-container won't add any DOM elements, so it shouldn't mess with the styles.

@pciavald
Copy link
Contributor Author

package-lock.json is included, only yarn.lock was removed because that's not the package manager you mention to use.

@UnchartedBull
Copy link
Owner

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

@pciavald pciavald marked this pull request as draft March 26, 2021 16:18
@pciavald pciavald changed the title i18n + FR (WIP) i18n + FR Mar 26, 2021
Copy link
Owner

@UnchartedBull UnchartedBull left a 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?

angular.json Show resolved Hide resolved
helper/locale.js Outdated Show resolved Hide resolved
helper/locale.js Outdated Show resolved Hide resolved
helper/locale.js Outdated Show resolved Hide resolved
helper/locale.js Outdated
if (err) throw new Error(err.message);

// hard copy of messages.xlf
const newTranslation = JSON.parse(JSON.stringify(extracted));
Copy link
Owner

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

Copy link
Contributor Author

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/locale/messages.fr.xlf Show resolved Hide resolved
@pciavald
Copy link
Contributor Author

pciavald commented Apr 7, 2021

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?

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.

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?

Not yet but it can be easily implemented, it's just a matter of changing the url to /lang/index.html.

@UnchartedBull
Copy link
Owner

UnchartedBull commented Apr 7, 2021

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.

@pciavald
Copy link
Contributor Author

pciavald commented Apr 8, 2021

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

@UnchartedBull
Copy link
Owner

Probably going to be flex layout.

@UnchartedBull
Copy link
Owner

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 :)

@pciavald
Copy link
Contributor Author

pciavald commented Apr 9, 2021

ready to merge once you re-generate package-lock

@UnchartedBull
Copy link
Owner

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 :)

Copy link
Owner

@UnchartedBull UnchartedBull left a comment

Choose a reason for hiding this comment

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

🚢

@UnchartedBull UnchartedBull merged commit 89aec2b into UnchartedBull:main Apr 11, 2021
@pciavald pciavald added this to the v3.0.0 milestone May 3, 2021
pciavald added a commit to pciavald/OctoDash that referenced this pull request May 12, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localize Text
2 participants