-
Notifications
You must be signed in to change notification settings - Fork 147
fix(translations): Add missing languages #1396
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
Conversation
Reviewer's GuideRefactors the multi-language selection widget from a checkbox list to a badge+dropdown picker, wires the custom widget into event forms, and synchronizes the configured language list and metadata with Weblate including new languages and corrected Chinese/German labels and codes. Sequence diagram for the new multi-language badge+dropdown interactionsequenceDiagram
actor User
participant LanguagePickerUI
participant HiddenSelect
participant DefaultLocaleSelect
participant Server
User->>LanguagePickerUI: Load event edit form
LanguagePickerUI->>HiddenSelect: Read initial selected options
LanguagePickerUI->>LanguagePickerUI: Render badges for selected languages
LanguagePickerUI->>LanguagePickerUI: Show placeholder if none selected
User->>LanguagePickerUI: Choose language from dropdown
LanguagePickerUI->>HiddenSelect: Mark corresponding option as selected
LanguagePickerUI->>LanguagePickerUI: Add language badge
LanguagePickerUI->>LanguagePickerUI: Hide/disable chosen option in dropdown
User->>LanguagePickerUI: Click remove button or badge
LanguagePickerUI->>HiddenSelect: Unselect corresponding option
LanguagePickerUI->>LanguagePickerUI: Remove badge
LanguagePickerUI->>LanguagePickerUI: Show option again in dropdown
LanguagePickerUI->>LanguagePickerUI: Show placeholder if no badges
LanguagePickerUI->>DefaultLocaleSelect: If removed language was default, switch to another selected value or empty
User->>LanguagePickerUI: Submit form
LanguagePickerUI->>Server: Send form data including hidden multiselect values
Server-->>User: Respond with updated event / validation result
Class diagram for MultipleLanguagesWidget and event form language fieldsclassDiagram
class CheckboxSelectMultiple {
}
class MultipleLanguagesWidget {
+MultipleLanguagesWidget(*args, **kwargs)
+sort()
}
class EventForm {
+event
+locales
+content_locales
+__init__(*args, **kwargs)
+clean()
}
CheckboxSelectMultiple <|-- MultipleLanguagesWidget
EventForm --> MultipleLanguagesWidget : uses_for_locales
EventForm --> MultipleLanguagesWidget : uses_for_content_locales
class LanguageConfigModule {
+ALL_LANGUAGES
+LANGUAGES_DATA
+LANGUAGES_OFFICIAL
+LANGUAGES_INCUBATING
+LANGUAGES_RTL
}
LanguageConfigModule ..> MultipleLanguagesWidget : provides_choices
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- In the multi_languages_select.html template, using
{{ _('No languages selected') }}and{{ _('Remove language') }}relies on_being in the template context, which typically isn’t guaranteed; consider using{% translate %}intodata-*attributes or rendering hidden elements and reading their text instead so the JS strings are reliably localized. - In the language selector script,
const selectedValues = () => ...is later shadowed byconst selectedValues = new Set(...)inside theif (defaultLocaleSelect)block, which is confusing and error‑prone; renaming one of these (e.g. togetSelectedValuesorselectedValueSet) would improve clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the multi_languages_select.html template, using `{{ _('No languages selected') }}` and `{{ _('Remove language') }}` relies on `_` being in the template context, which typically isn’t guaranteed; consider using `{% translate %}` into `data-*` attributes or rendering hidden elements and reading their text instead so the JS strings are reliably localized.
- In the language selector script, `const selectedValues = () => ...` is later shadowed by `const selectedValues = new Set(...)` inside the `if (defaultLocaleSelect)` block, which is confusing and error‑prone; renaming one of these (e.g. to `getSelectedValues` or `selectedValueSet`) would improve clarity.
## Individual Comments
### Comment 1
<location> `app/eventyay/orga/templates/orga/widgets/multi_languages_select.html:109-110` </location>
<code_context>
+ const hiddenSelect = container.querySelector('[data-role="hidden-select"]');
+ const selectedList = container.querySelector('[data-role="selected-list"]');
+ const defaultLocaleSelect = document.querySelector('select[name$="locale"]');
+ const emptyText = "{{ _('No languages selected')|escapejs }}";
+ const removeLabel = "{{ _('Remove language')|escapejs }}";
+
+ const escapeCss = (value) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** The use of `_()` in the template JS is likely undefined, producing empty strings instead of translations.
In Django templates, `{% load i18n %}` gives you `{% translate %}` / `{% trans %}`, but not a global `_()` function, so `{{ _('...') }}` will resolve to an undefined context variable and render as an empty string.
To keep these strings translated, use the template tags instead, for example:
```html+django
const emptyText = "{% translate 'No languages selected' as no_lang %}{{ no_lang|escapejs }}";
const removeLabel = "{% translate 'Remove language' as remove_lang %}{{ remove_lang|escapejs }}";
```
or, if you don’t need the intermediate variable:
```html+django
const emptyText = "{% translate 'No languages selected' %}";
const removeLabel = "{% translate 'Remove language' %}";
```
(add `escapejs` where appropriate).
</issue_to_address>
### Comment 2
<location> `app/eventyay/eventyay_common/forms/event.py:62-65` </location>
<code_context>
self.event = kwargs['obj']
super().__init__(*args, **kwargs)
+ # Ensure the language selectors use the custom dropdown widget even if defaults are not picked up elsewhere.
+ for fname in ('locales', 'content_locales'):
+ if fname in self.fields:
+ self.fields[fname].widget = MultipleLanguagesWidget(choices=self.fields[fname].choices)
if self.event and 'content_locales' in self.fields:
self.fields['content_locales'].initial = self.event.content_locales
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Reassigning the widget drops any existing widget attributes such as custom data-* or CSS classes.
Wrapping the existing field in `MultipleLanguagesWidget` drops any widget-level configuration beyond `choices` (e.g. `attrs`, custom `id`, `data-*` hooks, CSS classes). To avoid breaking existing customizations, you can carry those over when swapping the widget:
```python
for fname in ("locales", "content_locales"):
if fname in self.fields:
old_widget = self.fields[fname].widget
self.fields[fname].widget = MultipleLanguagesWidget(
choices=self.fields[fname].choices,
attrs=getattr(old_widget, "attrs", None),
)
```
This preserves field-specific attributes while using the new widget type.
```suggestion
# Ensure the language selectors use the custom dropdown widget even if defaults are not picked up elsewhere.
# Preserve existing widget attributes (e.g. custom data-*, CSS classes, IDs) when swapping widgets.
for fname in ('locales', 'content_locales'):
if fname in self.fields:
old_widget = self.fields[fname].widget
self.fields[fname].widget = MultipleLanguagesWidget(
choices=self.fields[fname].choices,
attrs=getattr(old_widget, 'attrs', None),
)
```
</issue_to_address>
### Comment 3
<location> `app/eventyay/config/settings.py:481-487` </location>
<code_context>
+ ('th', _('Thai')),
('tr', _('Turkish')),
- ('uk', _('Ukrainian')),
+ ('ua', _('Ukrainian')),
+ ('vi', _('Vietnamese')),
+ ('zh-hans', _('Chinese (Simplified)')),
</code_context>
<issue_to_address>
**question (bug_risk):** Using `ua` instead of the standard Django language code `uk` for Ukrainian may break locale resolution.
Django’s language code for Ukrainian is `uk`, but this change switches the key to `ua` without any mapping back to `uk`. If other parts of the system or Django’s i18n expect `uk` (e.g. locale dir names, `LANGUAGE_CODE`, browser `Accept-Language`), Ukrainian translations may no longer resolve.
If `ua` is needed externally, consider keeping the internal key / `public_code` / `path` as `uk` and only exposing `ua` where required, or use `uk` as the canonical key and add a dedicated mapping from `ua` → `uk`. Also verify this against your locale directory structure and language negotiation settings.
</issue_to_address>
### Comment 4
<location> `app/eventyay/orga/templates/orga/widgets/multi_languages_select.html:160-169` </location>
<code_context>
+ const selectedValues = () =>
</code_context>
<issue_to_address>
**suggestion:** The `selectedValues` function is shadowed by a local `selectedValues` constant, which is confusing and error-prone.
The top-level helper:
```js
const selectedValues = () =>
Array.from(hiddenSelect.querySelectorAll('option'))
.filter((opt) => opt.selected)
.map((opt) => opt.value);
```
is later shadowed inside the `if (defaultLocaleSelect)` block with:
```js
const selectedValues = new Set(
Array.from(hiddenSelect.querySelectorAll('option'))
.filter((opt) => opt.selected)
.map((opt) => opt.value)
);
```
This reuses the same name for a different type (`Set` vs function), which hurts readability and can cause mistakes if this block is refactored. Please rename the inner variable (e.g. `selectedValueSet` or `selectedLocales`) to avoid shadowing and make the intent clearer.
Suggested implementation:
```
const selectedValues = () =>
Array.from(hiddenSelect.querySelectorAll('option')).filter((opt) => opt.selected).map((opt) => opt.value);
```
```
if (defaultLocaleSelect) {
const selectedValueSet = new Set(
Array.from(hiddenSelect.querySelectorAll('option'))
.filter((opt) => opt.selected)
.map((opt) => opt.value)
);
// ... code that uses selectedValueSet ...
}
```
1. In the same `if (defaultLocaleSelect)` block, update all usages of the shadowing constant name from `selectedValues` to `selectedValueSet`. For example, change `selectedValues.has(locale)` to `selectedValueSet.has(locale)`, and any other reads/writes accordingly.
2. Verify there are no other inner scopes where a variable named `selectedValues` is declared; if there are, rename them similarly to avoid further shadowing and keep naming consistent with their actual type/intent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
app/eventyay/orga/templates/orga/widgets/multi_languages_select.html
Outdated
Show resolved
Hide resolved
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.
Pull request overview
This PR adds missing languages from Weblate to the UI and improves the language selection interface by replacing checkboxes with a more user-friendly badge+dropdown component.
- Adds 19 missing languages (Bulgarian, Catalan, Danish, Hungarian, Malay, Norwegian Bokmål, Romanian, Sinhala, Slovenian, Swedish, Thai, Vietnamese, etc.)
- Updates Chinese language labels from "Simplified Chinese" and "Traditional Chinese (Taiwan)" to standardized "Chinese (Simplified)" and "Chinese (Traditional)"
- Replaces checkbox-based language selection UI with an interactive badge+dropdown widget
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| multi_languages_select.html | Complete rewrite of the template to implement badge+dropdown UI with JavaScript interactions |
| widgets.py | Updates CSS class name from 'form-check-languages' to 'multi-language-select' |
| event.py | Ensures language selector fields use the new MultipleLanguagesWidget |
| settings.py | Adds missing languages to ALL_LANGUAGES list and LANGUAGES_INFO dictionary; corrects Chinese labels and Persian natural name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #1390
This PR makes the following changes:
Summary by Sourcery
Replace the multi-language event form widget with a badge-and-dropdown selector and expand and correct the available language definitions.
New Features:
Bug Fixes:
Enhancements: