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 support #78

Open
luismanson opened this issue Sep 3, 2022 · 47 comments
Open

i18n support #78

luismanson opened this issue Sep 3, 2022 · 47 comments
Assignees

Comments

@luismanson
Copy link
Contributor

Hi, I'm the guy who asked about translation support in Reddit. Would you consider a PR with some support? Frontend maybe.. I think I can add some basic implementation of react-i18n...

@bjarneo
Copy link
Member

bjarneo commented Sep 3, 2022

👋

Please do. Hopefully the frontend code is not to spaghetti. I know strings are thrown around all the places. Even with ternary operators.

react-i18n is 👍 to me!

@luismanson
Copy link
Contributor Author

Hey, What do you think? luismanson@563ffbf
it's not very complete, but the basic front strings.

@bjarneo
Copy link
Member

bjarneo commented Sep 6, 2022

Hey @luismanson, I will look into it today :)

@bjarneo
Copy link
Member

bjarneo commented Sep 6, 2022

@luismanson added a comment: luismanson@563ffbf#commitcomment-83183511

To me it looks great. You do not have to add the loader component for the Suspense, but if you do it would be cool. Feel free to open a PR :)

@bjarneo
Copy link
Member

bjarneo commented Sep 6, 2022

PR: #82

@bjarneo
Copy link
Member

bjarneo commented Sep 6, 2022

new PR: #83

Thank you for your contribution :)

@bjarneo bjarneo closed this as completed Sep 6, 2022
@bytebone
Copy link
Contributor

bytebone commented Sep 8, 2022

How would I go about adding more languages? I've created a new file public/locales/de/translation.json with the updated strings. Do I need to add this new language in src/i18n.js as well? And does the site itself already have some sort of system language detection or would that still be needed?

@luismanson
Copy link
Contributor Author

Hello, you need to set de as the default language in src/i18n.js.
Language switch/detection has not implemented yet.

PS: @bjarneo I'll submit my translation in the next day or so.

@bytebone
Copy link
Contributor

bytebone commented Sep 8, 2022

If i can figure out at what point the strings are loaded, maybe I can whip up a language detection & automatic switching method. Wish me luck 🤞🏼

@luismanson
Copy link
Contributor Author

check this doc: https://react.i18next.com/latest/using-with-hooks the example has some comments about language detection and file loading

@bjarneo bjarneo reopened this Sep 8, 2022
@bjarneo
Copy link
Member

bjarneo commented Sep 8, 2022

Reopening this issue.

One thing to consider here is to have an option to set the default language for the instance. This can be solved by adding an env var called SECRET_LANGUAGE. Then add than env var to the client config here: https://github.com/HemmeligOrg/Hemmelig.app/blob/main/config/default.cjs#L63

Which means you can fetch it like this: https://github.com/HemmeligOrg/Hemmelig.app/blob/main/src/client/routes/home/index.js#L51
One important note here is that the config that is imported and used in the frontend, is coming from this file: https://github.com/HemmeligOrg/Hemmelig.app/blob/main/src/client/config.js

@bjarneo
Copy link
Member

bjarneo commented Sep 8, 2022

Btw, so cool that you guys want to contribute 🎉

@bjarneo
Copy link
Member

bjarneo commented Sep 8, 2022

I have added you both @luismanson and @RainerZufahl as contributors to this repository. Let me know if something is not right about the settings. Should be allowed to create branches and PRs

@bytebone
Copy link
Contributor

bytebone commented Sep 8, 2022

Thank you! I've already created a fork of the repo which I'm working in, should I scrap that and create a new branch in this repo?

@bjarneo
Copy link
Member

bjarneo commented Sep 8, 2022

@rainerzufahl that is up to you, will work either way

@luismanson
Copy link
Contributor Author

Wow, thanks! I'll test if works with the spanish json later today.

@bytebone
Copy link
Contributor

bytebone commented Sep 9, 2022

automatic language detection & switching is now present via #88.

@luismanson
Copy link
Contributor Author

How should we handle new strings? I think this has to be as transparent as possible.
I noticed later that new strings could be written as t('some_string', 'Some String') but there are other options as Runtime Extraction and some plugins.

@bjarneo
Copy link
Member

bjarneo commented Sep 11, 2022

Hm. Interesting. I have to look into this.

But, of course, using t('some.string', 'Default string') is a good start, because that means if a language does not have the translation, it will automatically fallback to the default english string?

@luismanson
Copy link
Contributor Author

Yes, it should. If i understand correctly, this would even work with an empty default LANG setting.

@bjarneo
Copy link
Member

bjarneo commented Sep 12, 2022

I think that would be the first approach for this. How would runtime extraction and plugins work with create-react-app?

@luismanson
Copy link
Contributor Author

I'll have to look into it, i do not know tbh.

@bytebone
Copy link
Contributor

But, of course, using t('some.string', 'Default string') is a good start, because that means if a language does not have the translation, it will automatically fallback to the default english string?

That would mean that the english translations are hardcoded into the program, instead of an external string file, correct? While practical in some ways, that would make adjusting the english strings later on more complicated again.

@luismanson
Copy link
Contributor Author

Yes, that's right, it's a "good start" as @bjarneo called it and we can all agree that hardcoding stuff is a big no-no...
However this would allow us to still translate the app without adding too much work at the default strings or new code, this is where some for of extraction/tool could come in handy.

@ltguillaume
Copy link
Contributor

ltguillaume commented Sep 17, 2023

Continuing from #212

  • Currently, SECRET_FORCED_LANGUAGE=?? does not result in the language changing.
  • It's fine that there's an option to force the language, but imho this should not be the default behavior. Instead:
    1. Don't set SECRET_FORCED_LANGUAGE by default in docker-compose.yml. If set, use the defined locale.
    2. Use the browser's preferred language list to determine the language
    3. Fall back to English
  • I tried to force a language by changing the fallback language in i18n.js, but that doesn't work either. I think the entire i18next logic is faulty at this time.

@ltguillaume
Copy link
Contributor

@bjarneo Any luck trying to find out what's causing this? It's pretty hard to onboard people if there's a language barrier (especially with the knowledge that the translation is in fact already present 😅).

@bjarneo
Copy link
Member

bjarneo commented Nov 15, 2023

@ltguillaume this PR solves the SECRET_FORCED_LANGUAGE issue: 1404f0a

Test with German:
Screenshot 2023-11-15 at 07 43 30

@ltguillaume
Copy link
Contributor

Confirmed working. Next step, proper detection of the browser locale.

@bjarneo
Copy link
Member

bjarneo commented Nov 15, 2023

Yeah, that would've been something

@ltguillaume
Copy link
Contributor

ltguillaume commented Nov 15, 2023

Yeah, that would've been something

Well, when I looked at the code a while back, it seemed like the logic to do that is already there, but perhaps it doesn't actually get the values from request header to properly evaluate them against the available languages, thus defaulting to English (or SECRET_FORCED_LANGUAGE)?

@ltguillaume
Copy link
Contributor

Any progress with this? Because people can translate all they want, but it doesn't work anyway...

@bjarneo
Copy link
Member

bjarneo commented Jan 22, 2024

No, I have not looked into this.

@ltguillaume
Copy link
Contributor

ltguillaume commented Jan 22, 2024

I'm sorry I can't be of any help. I've tried to fix the code at least three times now, but I don't really understand how the i18n works with regard to Hemmelig. As a result, I keep having to make assumptions and so far, nothing has worked.

@bjarneo
Copy link
Member

bjarneo commented Jan 22, 2024

Screenshot 2024-01-22 at 20 49 37 https://github.com/HemmeligOrg/Hemmelig.app/commit/1a9d84e5de509ad46fdcef3ef9b32029225159cb

new release coming soon with this working based off of the browser language

@bjarneo
Copy link
Member

bjarneo commented Jan 22, 2024

This is in production right now:
Screenshot 2024-01-22 at 21 02 50

@bjarneo bjarneo closed this as completed Jan 22, 2024
@ltguillaume
Copy link
Contributor

That's great, thank you so much! 😁😁

@bjarneo
Copy link
Member

bjarneo commented Jan 22, 2024

@ltguillaume no worries. Let me know if it does not work as intended. :)

@ltguillaume
Copy link
Contributor

ltguillaume commented Jan 22, 2024

1a9d84e
That was it? Damn, now I feel stupid 😛 Gotta say, the documentation is a bit fragmented and together with tutorials I found it was all quite contradictory, but I'm happy it's fixed. Just tested and it works nicely now!

By the way, I found the following:

  1. userInfo.error, secrets.error, adminSettings.error and users.error are all absent in the translation.json files and all default to Not logged in. Should this be merged into a single error string, or do these errors still need to be made more specific in the code?
  2. Separate the link and decryption key, Secret URL without decryption key and Decryption key aren't translated.
  3. All the secret.* when viewing a secret are not translated in the UI (even though the translations are available). So the translations only seem to be used in one page.
  4. Most things in the (server and account) settings are still hardcoded English

@bjarneo
Copy link
Member

bjarneo commented Jan 23, 2024

Yes, that was it. Do not be harsh on yourself. It still has been implemented in a way not needed for Hemmelig as far as I can tell.

As for your bullet points, that is something that has to be looked into.

  1. Not sure. Have to look into this
  2. Then it should be added as translatable strings
  3. Should be an fairly easy fix
  4. This should be refactored to return status codes, and let the frontend handle the strings, which again is translateable

@ltguillaume
Copy link
Contributor

ltguillaume commented Jan 23, 2024

  1. See PR
  2. Aha, there seems to be a routing issue when fetching the translation file: instead of the language code (e.g. nl), it uses the path (e.g. secret).

In /secret:

Failed to load resource: /locales/secret/translation.json:1 
the server responded with a status of 404 ()

In /sigin:

GET https://hemmelig.my.server/locales/signin/translation.json 404 (Not Found)
MO @ index-d68b782a.js:53
AO @ index-d68b782a.js:53
value @ index-d68b782a.js:53
(anonymous) @ index-d68b782a.js:53
Promise.then (async)
value @ index-d68b782a.js:53
value @ index-d68b782a.js:53
value @ index-d68b782a.js:53
value @ index-d68b782a.js:53
(anonymous) @ index-d68b782a.js:53
value @ index-d68b782a.js:53
value @ index-d68b782a.js:53
value @ index-d68b782a.js:53
d @ index-d68b782a.js:53
value @ index-d68b782a.js:53
h @ index-d68b782a.js:53
setTimeout (async)
value @ index-d68b782a.js:53
(anonymous) @ index-d68b782a.js:53

@bjarneo
Copy link
Member

bjarneo commented Jan 24, 2024

Right, that makes sense. It should be an easy fix, I do not know the fix, but it should exclude /signin. Probably just a setting somewhere.

@ltguillaume
Copy link
Contributor

Right, that makes sense. It should be an easy fix, I do not know the fix, but it should exclude /signin. Probably just a setting somewhere.

Not just that. It seems to put the route secret/sigin where the language code should be.

@bjarneo
Copy link
Member

bjarneo commented Jan 24, 2024

Yeah, and this application do not support this kind of routing (yet), so I am unsure if it has ever worked for these routes.

@bjarneo bjarneo reopened this Jan 24, 2024
@ltguillaume
Copy link
Contributor

Wait, this is probably just because of detectionMethod = ['path'. Just remove path from the array.

@bjarneo
Copy link
Member

bjarneo commented Jan 24, 2024

Yes, I was thinking about removing this, but forgot to try to do so while fixing the navigator. Removed it now, and looks like that solved it: be6ba9e
Creating a new release. Great catch!

@bjarneo
Copy link
Member

bjarneo commented Jan 25, 2024

@ltguillaume v5.16.6

This contains the removal of path.

@ltguillaume
Copy link
Contributor

ltguillaume commented Jan 25, 2024

@ltguillaume v5.16.6

This contains the removal of path.

Yes, I tested it already and it works nicely 🙂

Now everything that faces "normal" end users (create and view secrets) is translatable 😃 except for the errors like Secret not found or Wrong password!.

Next is "sign up", "sign in", then the "account" part. These strings are also still hardcoded.

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

No branches or pull requests

4 participants