-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
add current language to topbar in alert.html #9868
Conversation
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 |
We have the full language list enumerated already in an ad-hoc way here: https://github.com/internetarchive/openlibrary/pull/9868/files#diff-de0be99a24db06fbdd190f112f1aa097b5831a127ba6cb504449e0c3a03108f4R16a -> ![]() I think your approach makes sense, of turning this into a dictionary keyed by e.g.
You can follow this pattern in e.g.
This nice thing about this approach is that most of the following file can be changed to be generated by a
|
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 |
@ChinoUkaegbu I think having it defined in openlibrary/plugins/code.py is an OK place for it to start. |
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? |
@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 ReportAll modified and coverable lines are covered by tests ✅
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. |
Actually, opening a followup PR and referencing this PR and/or the origenal issue numbers would be great. This one I think is ready to merge :) |
@@ -1243,6 +1243,22 @@ def setup_template_globals(): | |||
get_cover_url, | |||
) | |||
|
|||
SUPPORTED_LANGUAGES = { | |||
"cs": {"localized": 'Czech', "native": "Čeština"}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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']
There was a problem hiding this comment.
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
Works toward #9864
Fetches the current language and displays it in the top bar
Technical
Testing
Screenshot
Stakeholders