Skip to content

welcome-screen: Make various UI strings translatable #775

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

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

pwithnall
Copy link
Contributor

Previously they were hard-coded as English.

Signed-off-by: Philip Withnall [email protected]

Fixes: #770

@pwithnall pwithnall requested a review from starnight August 29, 2023 14:49
@pwithnall pwithnall self-assigned this Aug 29, 2023
@pwithnall
Copy link
Contributor Author

I haven’t actually tested this as the welcome screen isn’t used in the local webserver version of EK. It needs testing within kolibri-installer-android to check it works.

Since the welcome-screen is not run within the Kolibri JS environment (as I understand it), it might need additional code to be added to load the translations into vue-intl (Kolibri’s webpack hooks normally do that for us).

In the meantime, though, adding the $tr calls should not hurt unless I’ve added a syntax error.

@pwithnall pwithnall force-pushed the 770-welcome-screen-translation branch from 729316f to 8565a3e Compare August 29, 2023 15:12
@dylanmccall
Copy link
Collaborator

dylanmccall commented Aug 29, 2023

I like to test the welcome screen app with this lovely chain of container commands…

toolbox run pipenv run yarn run lerna run serve -- --scope=welcome-screen -- --port=5000

… which starts a web server at http://localhost:5000 with the welcome screen app. We can get it to show certain things by running commands from the Javascript console like WelcomeApp.showWelcome().

Copy link
Collaborator

@dylanmccall dylanmccall left a comment

Choose a reason for hiding this comment

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

At the moment, this fails with errors like:

[Vue warn]: Error in created hook: "TypeError: this.$tr is not a function"

found in

---> <App> at src/App.vue
       <Root> vue.runtime.esm.js:619
    VueJS 19
    <anonymous> main.js:57
    js app.js:4232
    __webpack_require__ app.js:849
    fn app.js:151
    1 app.js:4725
    __webpack_require__ app.js:849
    checkDeferredModules app.js:46
    <anonymous> app.js:925
    <anonymous> app.js:928

To solve that, I think we need to run (+ we might need a copy of) Kolibri's i18nSetup() function, which, if we're lucky, might just work! It defines Vue.prototype.$tr() among (many) other things. Here is where that gets run in Kolibri usually:

https://github.com/learningequality/kolibri/blob/v0.16.0-beta4/kolibri/core/assets/src/core-app/index.js#L72

And I don't completely understand how that finds its way to our channel apps (in their own iframes), but I assume it's this bit:

https://github.com/endlessm/kolibri-explore-plugin/blob/v6.40.0/packages/template-ui/src/main.js#L32-L36

@pwithnall
Copy link
Contributor Author

To solve that, I think we need to run (+ we might need a copy of) Kolibri's i18nSetup() function

That makes sense.

I tried implementing it, and have run into the same problem as with ek-components: I can’t import kolibri.utils.i18n in welcome-screen, even though it should be importable because it’s exported by Kolibri and is used in the main workspace of kolibri-explore-plugin.

I’m stuck!

@pwithnall pwithnall force-pushed the 770-welcome-screen-translation branch from 8565a3e to b01490e Compare September 4, 2023 17:03
@pwithnall pwithnall marked this pull request as draft September 4, 2023 17:05
@pwithnall
Copy link
Contributor Author

pwithnall commented Sep 4, 2023

Made some more progress on this today by rebasing on the ek-components PR so that welcome-screen can correctly import kolibri.utils.i18n. Now have hit another problem, which I’ll continue to look at tomorrow. (The Vue.prototype.$tr doesn’t seem to be being set properly.)

@pwithnall pwithnall force-pushed the 770-welcome-screen-translation branch from b01490e to 5f00a76 Compare September 5, 2023 13:12
@pwithnall pwithnall marked this pull request as ready for review September 5, 2023 13:12
@pwithnall
Copy link
Contributor Author

OK, this is now ready to review. It still needs to be based on #777, so that ideally needs to be reviewed first.

The problems with Vue.prototype.$tr not working turned out to have been caused by two copies of Vue being bundled, and the prototype being set on the wrong one. I’ve pushed a commit which uses resolutions to force only one copy of Vue to be bundled.

@pwithnall pwithnall force-pushed the 770-welcome-screen-translation branch from 5f00a76 to 3ec16f6 Compare September 5, 2023 13:36
@pwithnall
Copy link
Contributor Author

Great, the CI linter now thinks that a load of files I did not touch are not pretty enough, but declines to give suggestions on how they are not pretty enough.

I give up on this for the moment.

@dbnicholson
Copy link
Member

Great, the CI linter now thinks that a load of files I did not touch are not pretty enough, but declines to give suggestions on how they are not pretty enough.

I give up on this for the moment.

Isn't the actual problem:

Error: ENOENT: no such file or directory, open '/home/runner/work/kolibri-explore-plugin/kolibri-explore-plugin/kolibri/core/assets/src/core-app/apiSpec.js'

@pwithnall
Copy link
Contributor Author

I don’t think so. I see that error on every build and it doesn’t seem to affect things. I could be wrong though.

@dbnicholson
Copy link
Member

Can you try running yarn run lint-frontend locally? Or yarn run lint-frontend:format to have it write out the desired changes. That's what pre-commit is doing. In my experience, pre-commit has trouble figuring out "what's changed" when you rebase commits. I don't know if that's what you're doing, but you could do something like git rebase -i -x 'yarn run lint-frontend:write' to have it check between each commit.

pwithnall added a commit that referenced this pull request Sep 8, 2023
And that new kolibri-tools has not been released yet. The linting can
hopefully be re-enabled when it has been.

See #775 (comment)

Signed-off-by: Philip Withnall <[email protected]>
@pwithnall
Copy link
Contributor Author

Note for anyone thinking about merging this: the branch is based on #777, so that needs to be reviewed and landed first.

@dbnicholson
Copy link
Member

Since #793, welcome-screen has become loading-screen and is completely decoupled from the rest of the code, so this will have to be reworked. @manuq suggested using vue-intl directly, which seems like a good idea. However, I don't know how that will play with the rest of the i18n infrastructure. Possibly it's easier to use kolibri-tools for consistency.

@pwithnall
Copy link
Contributor Author

I’ll look at rebasing and reworking this tomorrow.

@dbnicholson
Copy link
Member

I think the strings that got moved into the plugin should be straightforward to pull into the regular kolibri i18n pipeline. The standalone app has about 5 strings left and likely needs a little thought. The formatjs process looks pretty straightforward, but it would add a 3rd i18n pipeline to the mix.

@pwithnall
Copy link
Contributor Author

OK, I’ve completely reworked this to use formatjs directly, which means its message catalogues are completely separate from everything else, but since they only contain a handful of strings it’s probably fine. (Well, I can’t think of a better idea.)

@manuq
Copy link
Collaborator

manuq commented Sep 19, 2023

Could Latinamerica Spanish be part of this PR too? Basically copy es_ES to es_419.

@pwithnall
Copy link
Contributor Author

Could Latinamerica Spanish be part of this PR too? Basically copy es_ES to es_419.

I’d prefer to not block the merging of this on anything which is not strictly necessary for bootstrapping translations in loading-screen.

@manuq
Copy link
Collaborator

manuq commented Sep 19, 2023

Could Latinamerica Spanish be part of this PR too? Basically copy es_ES to es_419.

I’d prefer to not block the merging of this on anything which is not strictly necessary for bootstrapping translations in loading-screen.

Sounds fair! Just a reminder that Spain is not our main audience as far as I understand.

@pwithnall pwithnall force-pushed the 770-welcome-screen-translation branch from 8f78beb to 4d73171 Compare September 20, 2023 09:42
@pwithnall
Copy link
Contributor Author

Sounds fair! Just a reminder that Spain is not our main audience as far as I understand.

Good point. I did s/es-ES/es-419/ as a quick improvement. My Spanish is equally bad for es-419 vs es-ES, so it doesn’t make the actual translation less or more appropriate.

Copy link
Member

@dbnicholson dbnicholson left a comment

Choose a reason for hiding this comment

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

This looks good. I had a few questions and suggestions, but I think this is fine to go in as is and will do what we need.

pwithnall and others added 4 commits September 20, 2023 16:58
We can’t use Kolibri’s internationalisation functions, as loading-screen
is standalone and executed outside the context of a Kolibri plugin. So
we use formatjs and its Vue integration.

See the new `README.md` file for full details.

I decided not to add support for `babel-plugin-formatjs` as it
duplicates checks already done by the `formatjs` CLI and would
cause the loading-screen babel setup to diverge from the babel setup for
the rest of the plugin.

References:
 - https://formatjs.io/docs/getting-started/application-workflow
 - https://formatjs.io/docs/intl/#formatmessage
 - https://formatjs.io/docs/vue-intl/#tooling
 - https://formatjs.io/docs/tooling/babel-plugin/
 - https://webpack.js.org/api/module-methods/
 - https://webpack.js.org/guides/code-splitting/#dynamic-imports

Signed-off-by: Philip Withnall <[email protected]>

Helps: #770
Previously they were hard-coded as English.

Signed-off-by: Philip Withnall <[email protected]>

Fixes: #770
These can now be used during compilation, or can be sent away for
translation.

The translated versions should be committed as
`packages/loading-screen/lang/es-ES.json`, etc., and the new language
code added to `supportedLocales` in `main.js` (in RFC 5646 format).

Signed-off-by: Philip Withnall <[email protected]>

Helps: #770
This is done using my rusty Spanish and an online translator, so might
not be very high quality.

Signed-off-by: Philip Withnall <[email protected]>

Helps: #770
@pwithnall pwithnall force-pushed the 770-welcome-screen-translation branch from 4d73171 to 3ccadfb Compare September 20, 2023 15:59
pwithnall added a commit that referenced this pull request Sep 20, 2023
We added them because that’s what Kolibri does, but it doesn’t really
gain us much. `yarn run i18n-django-compilemessages` doesn’t take long,
so we can run it on every build and avoid committing build products to
the repository.

See #775 (comment)

Signed-off-by: Philip Withnall <[email protected]>
@dbnicholson
Copy link
Member

Looks good. I was going to ask about distributing the compiled JSON, but I see that webpack seems to be including it.

@dbnicholson dbnicholson merged commit 6a006d7 into master Sep 20, 2023
@dbnicholson dbnicholson deleted the 770-welcome-screen-translation branch September 20, 2023 16:51
dbnicholson pushed a commit that referenced this pull request Sep 20, 2023
We added them because that’s what Kolibri does, but it doesn’t really
gain us much. `yarn run i18n-django-compilemessages` doesn’t take long,
so we can run it on every build and avoid committing build products to
the repository.

See #775 (comment)

Signed-off-by: Philip Withnall <[email protected]>
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.

Add translation calls to welcome-screen
4 participants