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

Internationalization of ZIM viewer #871

Merged
merged 12 commits into from
Feb 6, 2023
Merged

Internationalization of ZIM viewer #871

merged 12 commits into from
Feb 6, 2023

Conversation

veloman-yunkan
Copy link
Collaborator

@veloman-yunkan veloman-yunkan commented Jan 19, 2023

This PR has been extracted out of #846 so that changes related to ZIM viewer can be merged without waiting for resolution of the issues discovered while testing internationalization of the welcome page.

Depends on #869

In this PR I have eliminated the roundabout way of arriving at the final state of the internationalization utilities. Despite the title of this PR, it is mostly about introducing the infrastructure for frontend internationalization - ZIM viewer serves only as a small test range.

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Base: 72.01% // Head: 72.01% // No change to project coverage 👍

Coverage data is based on head (a620c86) compared to base (35aacf7).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #871   +/-   ##
=======================================
  Coverage   72.01%   72.01%           
=======================================
  Files          53       53           
  Lines        3745     3745           
  Branches     2093     2093           
=======================================
  Hits         2697     2697           
  Misses       1046     1046           
  Partials        2        2           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@veloman-yunkan veloman-yunkan force-pushed the zim_viewer_i18n branch 2 times, most recently from 9f71990 to 952a1f9 Compare January 20, 2023 14:36
@veloman-yunkan veloman-yunkan marked this pull request as ready for review January 20, 2023 14:44
@veloman-yunkan
Copy link
Collaborator Author

Cleaned-up portion of #846 related to reusable frontend internationalization utilities and their application in the context of ZIM viewer is ready for review and final testing.

Base automatically changed from userlang_cookie_fixes to main January 24, 2023 18:16
@veloman-yunkan veloman-yunkan force-pushed the zim_viewer_i18n branch 2 times, most recently from 193beee to 2af6974 Compare January 25, 2023 19:50
Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

Two small changes for the commit messages but else we are good. It works pretty well.

static/resources_list.txt Show resolved Hide resolved
static/skin/taskbar.css Show resolved Hide resolved
This is a quick workaround (at the expense of data duplication) for
having to generate the i18n data in JSON format from the embedded i18n
resource data.

Note, however, that at this point i18n resources are not included in
the list of regular static resources. This will change in the next
commit.
Did it by making the kiwix-compile-resources script take multiple
arguments.
Before this change, some of the actions related to the initialization of
the viewer were run in the global scope as a side effect of loading
/skin/viewer.js. This change moves those actions into setupViewer().
mustache.js was obtained from the following location:

- https://github.com/janl/mustache.js/raw/v4.2.0/mustache.js

mustache.min.js which is a build artifact was taken from

- https://cdnjs.cloudflare.com/ajax/libs/mustache.js/4.2.0/mustache.min.js

Note that mustache.js is included in order to comply with Debian packaging
requirements but will not be used in any other way (hence it is not
added to resources_list.txt).
Serving the language list as a JS file rather than JSON simplifies
a few things:

- cacheid management;
- having to manually delay the UI initialization until the JSON file
  is loaded.

static/skin/languages.js must be generated/updated manually by running
the static/generate_i18n_resources_list.py script.
ZIM viewer is now internally internationalized but the UI language
can only be set by providing the `userlang` query parameter in the URL:

Example:

  /viewer?userlang=fr#wikipedia_en_climate_change_mini_2021-03/A/index
         ^^^^^^^^^^^^
Known issues

- styling / placement

- language changes via the selector UI are not recorded in the
  navigation history

- changing the language via the UI doesn't update the `?userlang=` URL
  query parameter
Now that we have proper UI for user language selection, we don't need
the `?userlang=` query parameter present in the URL. If `?userlang=` is
explicitly provided in the URL, it sets the requested language and
disappears.
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

LGTM

@veloman-yunkan veloman-yunkan merged commit f3c0d5d into main Feb 6, 2023
@veloman-yunkan veloman-yunkan deleted the zim_viewer_i18n branch February 6, 2023 16:50
@veloman-yunkan
Copy link
Collaborator Author

This PR introduced another problem for the seamonkey browser caused by usage of import.meta in a620c86

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.

3 participants