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

Multi-language support #72

Closed
nesnoj opened this issue Aug 6, 2019 · 30 comments · Fixed by #97
Closed

Multi-language support #72

nesnoj opened this issue Aug 6, 2019 · 30 comments · Fixed by #97
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@nesnoj
Copy link
Member

nesnoj commented Aug 6, 2019

Text is prepared by @Stefanie08 in
https://github.com/rl-institut/WAM_APP_stemp_abw/tree/feature/multi_language_support

@4lm What options do we have to implement multi-language support?

@nesnoj nesnoj added the enhancement New feature or request label Aug 6, 2019
@nesnoj nesnoj assigned 4lm and nesnoj Aug 6, 2019
@nesnoj nesnoj added this to the finalization milestone Aug 6, 2019
@4lm
Copy link
Contributor

4lm commented Aug 6, 2019

Thanks for the task, I like 😺 As spoken f2f - I will have a look and report!

4lm added a commit that referenced this issue Aug 7, 2019
@4lm
Copy link
Contributor

4lm commented Aug 7, 2019

Hi @nesnoj,

multi-language works! In templates and views (also in the Charts 🚀). I had to do some workarounds, because multi-language support is normally tied as global state to wam/settings.py.
But we want each app to have a separate language state, so we are missing some features, which is OK IMO. The stuff we need is there!

I added a simple version with a (unstyled) language switcher in the navbar: 031e415

To test it out you have to add locale as middleware in wam/settings.py:

MIDDLEWARE = [
    'django.middleware.locale.LocaleMiddleware',
    ...
]

The last problem are the labels and texts in the config files in stemp_abw/config. They are set right at the beginning when the app server of the WAM is started. So right now it's not possible to set the language at runtime, load the appropriate config files with the appropriate language and reload the page or go to an app via link. That's why we always have to restart the server, when we change some stuff in the config files ...

Maybe I find a doable solution for the config files tomorrow ...

@nesnoj
Copy link
Member Author

nesnoj commented Aug 14, 2019

As agreed yesterday f2f, you create a draft to find out if all language-related parts can be covered.
Let me know if you need a review..

EDIT:
BTW: In the end, pls add deployment instructions to the dev docs.

4lm added a commit that referenced this issue Aug 15, 2019
4lm added a commit that referenced this issue Aug 15, 2019
4lm added a commit that referenced this issue Aug 15, 2019
4lm added a commit that referenced this issue Aug 15, 2019
@4lm
Copy link
Contributor

4lm commented Aug 15, 2019

Hi @nesnoj,

enabling I18N in stemp_abw is as good as done. As far as I can see, there are only some minor things left to do (e. g. the unit attributes in stemp/abw/layers_region.cfg and temp/abw/layers_region.cfg are in German and should also be translatable).

In the end I opted for the .cfg- and .md files to use the approach you first suggested. I did this, because it was way less work, than changing the code to go the locale .po files route all the way.

So, the way to add a language to stemp_abw is relatively simple now:

  1. Make a new locale folder with .po file via python manage.py message commands in the desired language
  2. Copy all .cfg and .md files and folders from a given locale language folder to the folder from the step before
  3. Translate
  4. Add the language name as string to the LANGUAGE_STORE list in app_settings of stemp_abw. This will also automatically add the language to the language switcher.

Next week, I can explain this approach in more detail f2f, if you like.

@nesnoj
Copy link
Member Author

nesnoj commented Aug 16, 2019

Sounds great, looking forward! :)

4lm added a commit that referenced this issue Aug 20, 2019
4lm added a commit that referenced this issue Aug 20, 2019
4lm added a commit that referenced this issue Aug 20, 2019
4lm added a commit that referenced this issue Aug 20, 2019
4lm added a commit that referenced this issue Aug 20, 2019
4lm added a commit that referenced this issue Aug 21, 2019
4lm added a commit that referenced this issue Aug 22, 2019
@nesnoj
Copy link
Member Author

nesnoj commented Aug 23, 2019

Good work @4lm!
I merged your PR #76.

There're some shortcomings, I'd be very happy if you could tackle them:

  1. The result chart tooltips (texts below the charts) are not translated. They are loaded correctly here but the class ResultChart is instantiated by each chart only once: at the start of the app in result_charts.py. Hence, the current locale texts are not loaded again. For the very same reason, you used callables in app_settings, I think this could be fixed the very same way here..
  2. I don't like the redundant layer files (layers_*.cfg) for each language, but I guess the units are the only reason y u opted for separate en files? What about moving the units to the labels file?
  3. There're no translation for the JS files (e.g. for loading, leave page warning, ...). Is it a hassle to add it? I know, no template tags available.. :(
  4. Warning message ("results lost..") on language switch (same as the other ones you used when switching to contect etc.)

I saw u added some structure for i18n to the docs, please complete this section :).

Branch 4lm/feature/add-multi-language-support is up-to-date, you can reuse it if you like...

Thanks a lot!

@nesnoj nesnoj modified the milestones: finalization, v1.0 Aug 27, 2019
@nesnoj
Copy link
Member Author

nesnoj commented Sep 2, 2019

Beside my comments above, I discovered a bug:

When switching from DE to EN, the first accordion in energy system is not expanded on startup as expected:

image

Contrary, when switching to DE, it is expanded properly..

@nesnoj
Copy link
Member Author

nesnoj commented Sep 18, 2019

@4lm Urgent:
I just deployed the app on WAMdev. When hitting "OK" after selecting a language, the redirect does not work correctly: I'm always redirected to the root (landing page) of the WAM instead of staying on the current page. There's a specific order in which Django tries to redirect when using the set_language()view (that u used).
Strangely, it works perfectly fine in my local environment.

@4lm
Copy link
Contributor

4lm commented Sep 19, 2019

@nesnoj, I think I see the problem, will do fix a branch!

@4lm
Copy link
Contributor

4lm commented Sep 19, 2019

@nesnoj, I also can't locally reproduce this issue, can you test if this commit 2e4acba on branch 'fix/broken-language-support` fixes the issue. The stemp_abw app now also uses 'de-DE' as language definition and in locale instead of just 'de'.

@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

Ok, will test now

4lm added a commit that referenced this issue Sep 19, 2019
@4lm
Copy link
Contributor

4lm commented Sep 19, 2019

@nesnoj, I added a language switch confirm, which is only active in the map view, but not on the start page 4d1d3fd. The code also has all commits from #93, so please review this pull request first. Will also do a pull request for this one.

Edit: Pull request for this one is #94

@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

Both merged and dev deployed on WAMdev
Still redirecting to WAM landing page... :(

@4lm
Copy link
Contributor

4lm commented Sep 19, 2019

Mhh, Ok, will have a look ...

@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

Locale Middleware moved as you proposed.
Same result..

@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

Maybe it's something with the webserver - caddy behaves differently than Django's (gunicorn?)
May try to set a redirect target explicitly to avoid the fallback mentioned above..

@4lm
Copy link
Contributor

4lm commented Sep 19, 2019

@nesnoj, can you test if this commit 6e5f7c7 fixes it, the code is on branch fix/redirect-issue: https://github.com/rl-institut/WAM_APP_stemp_abw/tree/fix/redirect-issue.

@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

Works out!! 🎉 🎉 🎉 🚀

Can you crate a PR (just for the log)

@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

Can you crate a PR (just for the log)

I'll do it
thx for this important fix

nesnoj added a commit that referenced this issue Sep 19, 2019
Explicitly set next value to the path the request originated from #72
@4lm
Copy link
Contributor

4lm commented Sep 19, 2019

🎉 🎉 🎉 Cool 😎

@4lm
Copy link
Contributor

4lm commented Sep 19, 2019

Here comes the next PR, I fixed the accordion #97. This one was easy 😉

nesnoj added a commit that referenced this issue Sep 19, 2019
…ior-of-accordion

Fix default area expand behavior of accordion #72
@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

ooops

@nesnoj nesnoj reopened this Sep 19, 2019
@nesnoj
Copy link
Member Author

nesnoj commented Sep 19, 2019

@4lm Last remaining task: docs

@nesnoj
Copy link
Member Author

nesnoj commented Sep 26, 2019

Can u finish this one today too?

@nesnoj nesnoj closed this as completed Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants