Add english translations to members page #512
Conversation
WalkthroughThe recent updates mainly focus on enhancing the localization and translation capabilities of a webshop project. Changes include the introduction of dynamic content in HTML templates using translation functions, updating Docker configurations to include additional libraries for localization, and modifications in TypeScript files to integrate translation hooks. The project now supports more inclusive language options, improving accessibility for users with different language preferences. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (21)
Files skipped from review due to trivial changes (7)
Additional comments not posted (23)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
amrsoll
force-pushed
the
english_translations
branch
from
2c7284d
to
e8be12e
Compare
amrsoll
force-pushed
the
english_translations
branch
from
e8be12e
to
85453df
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| `<h2>${t( | ||
| "change_pin.error", | ||
| )}</h2><b class="uk-text-danger"">${get_error(e)}</b>`, |
There was a problem hiding this comment.
Improve error handling presentation.
The error message presentation here could be improved for better user experience. Consider using a more structured layout or additional styling to make the error message clearer and more user-friendly.
- `<h2>${t("change_pin.error")}</h2><b class="uk-text-danger"">${get_error(e)}</b>`,
+ `<h2>${t("change_pin.error")}</h2><div class="uk-alert-danger" uk-alert><p>${get_error(e)}</p></div>`,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| `<h2>${t( | |
| "change_pin.error", | |
| )}</h2><b class="uk-text-danger"">${get_error(e)}</b>`, | |
| `<h2>${t( | |
| "change_pin.error", | |
| )}</h2><div class="uk-alert-danger" uk-alert><p>${get_error(e)}</p></div>`, |
| Du har inte satt någon PIN-kod ännu. Använd BYT-knappen för att | ||
| sätta den. PIN-koden används för{" "} | ||
| <a href={URL_MEMBERBOOTH}>memberbooth</a>. | ||
| {t("member_page.no_pin_warning")} |
There was a problem hiding this comment.
Address potential accessibility issues.
The color red is used directly in the style for indicating warning messages. Consider using a class that can be controlled via CSS for better accessibility and to support themes.
- <label class="uk-form-label" style="color: red;">
+ <label class="uk-form-label warning-label">And in your CSS file:
.warning-label {
color: red; /* Or use a more accessible color */
}
There was a problem hiding this comment.
Thanks! Great job!
Looks good for the most part, but with a few comments.
I'm not sure about using POT files. They look somewhat ancient.
Fluent seems like a more modern choice: https://github.com/projectfluent/python-fluent/blob/main/fluent.runtime/docs/usage.rst, but I haven't looked into this in depth.
They seem to be preferred by Mozilla: https://mozilla-l10n.github.io/documentation/localization/dev_best_practices.html
There also seems to be typescript support: https://github.com/macabeus/fluent-typescript (first result when googling).
Changing everything to use Fluent, POT, or some other system will be a lot of work, though. So it's probably best to get this PR merged before doing that.
| @@ -1,7 +1,7 @@ | |||
| <!doctype html> | |||
| <html> | |||
| <head> | |||
| <title>Stockholm Makerspace Webshop</title> | |||
| <title>Stockholm Makerspace {{ _("Webshop") }}</title> | |||
There was a problem hiding this comment.
Perhaps Stockholm Makerspace should be a localization string as well? To make it easier to re-use for different makerspaces.
There was a problem hiding this comment.
I was considering it, but what you actually want there is a variable holding the brand name.
| > | ||
| <a class="product-nav-right" href='{{ url("/cart") }}' | ||
| >Min kundvagn<span id="cart-sum"></span> | ||
| >{{ _("Shopping Cart") }}Min kundvagn<span |
There was a problem hiding this comment.
I think you left the original text here.
| @@ -461,7 +463,9 @@ async function change_phone_number( | |||
| ); | |||
| switch (r) { | |||
| case "ok": | |||
| await UIkit.modal.alert(`<h2>Telefonnummret är nu bytt</h2>`); | |||
| await UIkit.modal.alert( | |||
| `<h2>${t("change_phone.validation_changed_success")}</h2>`, | |||
There was a problem hiding this comment.
phone_number_changed_success?
| (only {days} day(s) left). Remember to extend your lab | ||
| membership before next Friday morning. | ||
| {t("member_page.labaccess.valid")(enddate, days)} | ||
| {t("member_page.labaccess.extend_on_friday")} |
There was a problem hiding this comment.
| {t("member_page.labaccess.extend_on_friday")} | |
| {t("member_page.labaccess.expires_within_days")} |
The Friday text was actually outdated.
There was a problem hiding this comment.
Should we rip out that string entirely instead?
| }, | ||
| addr: { | ||
| street_and_number: "Street and number", | ||
| extra: "Extra address info, fx C/O, 2.th.", |
There was a problem hiding this comment.
Is fx intended to stand for for example here? I don't think that's a common usage. Use e.g. instead.
Also, I'm not quite sure what 2.th. is supposed to mean.
There was a problem hiding this comment.
I know it isn't standard at all, I simply wanted to have it understandable for people living in Denmark. Unfortunately this type of info is mostly based on location, not language, so we can't find a standard
There was a problem hiding this comment.
For 2.th. I can accept that, but e.g. is the English abbreviation for "for example", not fx.
| @@ -557,6 +678,7 @@ export const useTranslation = (): Translator => { | |||
|
|
|||
| const heuristicallyPickLanguage = (): "en" | "sv" => { | |||
| const langs = navigator.languages || [navigator.language]; | |||
| console.log("lang ", langs); | |||
There was a problem hiding this comment.
This looks like a generated file. If so, it should be ignored and added so it's generated when building (if it isn't already).
It seems like the babel translation tool with .pot file(s) could unify the translation methods, what do you think? I will come with an extra commit to show what I mean
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores