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

add current language to topbar in alert.html #9868

Merged

Conversation

ChinoUkaegbu
Copy link
Contributor

Works toward #9864

Fetches the current language and displays it in the top bar

Technical

Testing

Screenshot

Screenshot (334)

Stakeholders

@ChinoUkaegbu
Copy link
Contributor Author

This currently displays the codes (e.g. 'es', 'de') which isn't exactly user friendly. My current plans for implementation are to have a dictionary that maps the code to the full language name so it displays Français (fr) instead of fr. Wondering if this is the correct way to go or if there's some other existing function that can be used to fetch the language in that format.

@mekarpeles mekarpeles self-assigned this Sep 11, 2024
@mekarpeles
Copy link
Member

mekarpeles commented Sep 15, 2024

We have the full language list enumerated already in an ad-hoc way here: https://github.com/internetarchive/openlibrary/pull/9868/files#diff-de0be99a24db06fbdd190f112f1aa097b5831a127ba6cb504449e0c3a03108f4R16a ->
https://github.com/internetarchive/openlibrary/blob/master/openlibrary/templates/languages/language_list.html

Screenshot 2024-09-15 at 9 46 35 AM

I think your approach makes sense, of turning this into a dictionary keyed by data-lang-id="cs"

e.g.

SUPPORTED_LANGUAGES = {
    "cs": {
        "localized": _('Czech'),
        "native": "Čeština"
    },
    "de": {
        "localized": _('German'),
        "native": "Deutsch"
    },
    "en": {
        "localized": _('English'),
        "native": "English"
    },
    "es": {
        "localized": _('Spanish'),
        "native": "Español"
    },
    "fr": {
        "localized": _('French'),
        "native": "Français"
    },
    "hr": {
        "localized": _('Croatian'),
        "native": "Hrvatski"
    },
    "it": {
        "localized": _('Italian'),
        "native": "Italiano"
    },
    "pt": {
        "localized": _('Portuguese'),
        "native": "Português"
    },
    "hi": {
        "localized": _('Hindi'),
        "native": "हिंदी"
    },
    "sc": {
        "localized": _('Sardinian'),
        "native": "Sardu"
    },
    "te": {
        "localized": _('Telugu'),
        "native": "తెలుగు"
    },
    "uk": {
        "localized": _('Ukrainian'),
        "native": "Українська"
    },
    "zh": {
        "localized": _('Chinese'),
        "native": "中文"
    }
}

You can follow this pattern in plugins/openlibrary/code.py to make SUPPORTED_LANUGAGES accessible to the templates:

https://github.com/internetarchive/openlibrary/blob/master/openlibrary/plugins/openlibrary/code.py#L1269

e.g.

  ...
 "supported_langs": SUPPORTED_LANGUAGES,
  ...

This nice thing about this approach is that most of the following file can be changed to be generated by a for loop:
https://github.com/internetarchive/openlibrary/blob/master/openlibrary/templates/languages/language_list.html

$def with (classes='')

<ul class="locale-options $classes">
  $for lang in supported_langs:
    <li><a href="#" lang="$lang" data-lang-id="$lang" title="$(lang['localized'])">$(lang['native']) ($lang)</a></li>
</ul>

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Sep 15, 2024
@ChinoUkaegbu
Copy link
Contributor Author

Great, I'll work on this now. Also just to confirm, would the dictionary be defined in https://github.com/internetarchive/openlibrary/blob/master/openlibrary/plugins/openlibrary/code.py or in a specific file and then referenced in plugins/openlibrary/code.py?

@mekarpeles
Copy link
Member

@ChinoUkaegbu I think having it defined in openlibrary/plugins/code.py is an OK place for it to start.

@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Sep 17, 2024
@ChinoUkaegbu
Copy link
Contributor Author

Currently looks like this:
Screenshot (345)

LMK if I should make any changes. I was using the Gitpod workspace and CSS changes don't seem to be reflected (seems to be a common issue) but when I pasted those values in the developer console, the changes were reflected so hopefully that works.

Also, for the language list refactoring, should that be a separate PR or should i just add another commit with the changes to this one?

@mekarpeles
Copy link
Member

@ChinoUkaegbu I think it's your choice -- the language refactor seems like a nice win and it may make sense as a separate commit to this PR, though if you prefer making another issue and PR for it, that's your call!

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.07%. Comparing base (ce16a79) to head (8068516).
Report is 431 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9868      +/-   ##
==========================================
+ Coverage   16.06%   17.07%   +1.01%     
==========================================
  Files          90       89       -1     
  Lines        4769     4720      -49     
  Branches      832      828       -4     
==========================================
+ Hits          766      806      +40     
+ Misses       3480     3405      -75     
+ Partials      523      509      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mekarpeles
Copy link
Member

Actually, opening a followup PR and referencing this PR and/or the original issue numbers would be great. This one I think is ready to merge :)

@mekarpeles mekarpeles merged commit b485525 into internetarchive:master Sep 18, 2024
4 checks passed
@@ -1243,6 +1243,22 @@ def setup_template_globals():
get_cover_url,
)

SUPPORTED_LANGUAGES = {
"cs": {"localized": 'Czech', "native": "Čeština"},
Copy link
Collaborator

@cdrini cdrini Sep 23, 2024

Choose a reason for hiding this comment

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

These will need to be wrapped with the _ in order to be actually localized, e.g.:

{"localized": _('Czech'), "native": "Čeština"}

Otherwise they will always be in English.

This also means we can't have this be a constant global, or else the _ will only read the user language once! So we'll want to make this into a function so that _ can get the user language per-request. E.g.

def get_supported_languages():
    return {
        "cs": {"localized": _('Czech'), "native": "Čeština"},
        ...
    }

And then make this function publicly accessible from the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! Initially when I tried to create the dictionary, it followed this format:

{"localized": _('Czech'), "native": "Čeština"}

but I kept getting an error message and so I left the dictionary in the format it currently is without the underscore with the intent of using the underscore in whatever page it's imported into. In PR #9889 I actually use the localization _ here and it works so I'm wondering if I should follow that format in this case or just have that get_supported_languages() dictionary with the localization embedded in

Copy link
Collaborator

@cdrini cdrini Sep 23, 2024

Choose a reason for hiding this comment

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

Ah you will have to import the _ in the python file to get access to it, with from openlibrary.i18n import gettext as _! That should do the trick. Was that maybe the error you were getting?

Unfortunately _ only works with static text, since essentially what happens is before the code is run, the python and html is analyzed and all _("...") are extracted and put into a separate file for our translators. This means that _ doesn't work with variables, only literal strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The import worked, thank you! I've addressed the concerns (hopefully) in #9906

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wonderful thank you! I responded there 😊

@@ -9,6 +9,7 @@
<div class="language-component header-dropdown">
<details>
<summary>
<span>$(supported_langs[get_lang() or 'en']['native']) ($(get_lang() or 'en'))</span>
Copy link
Collaborator

@cdrini cdrini Sep 23, 2024

Choose a reason for hiding this comment

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

This will error if the user language is not supported! Eg if get_lang() returns sq. Use the python .get to handle this case as well, eg

$ ui_lang = get_supported_languages().get(get_lang()) or get_supported_languages()['en']

Eg: https://staging.openlibrary.org/?lang=sq

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh note this also means we'll have to change what's inside the brackets, otherwise for unsupported languages (eg Albanian which has language code sq) we'll see something like: "English (sq)" which is incorrect

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.

4 participants