endlessm / kolibri-explore-plugin

The kolibri plugin to add the custom channel representation
MIT License
2 stars 4 forks source link

welcome-screen: Make various UI strings translatable #775

Closed pwithnall closed 1 year ago

pwithnall commented 1 year ago

Previously they were hard-coded as English.

Signed-off-by: Philip Withnall philip@tecnocode.co.uk

Fixes: #770

pwithnall commented 1 year ago

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.

dylanmccall commented 1 year ago

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().

pwithnall commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

dbnicholson commented 1 year ago

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 commented 1 year ago

I’ll look at rebasing and reworking this tomorrow.

dbnicholson commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

pwithnall commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

dbnicholson commented 1 year ago

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