-
Notifications
You must be signed in to change notification settings - Fork 23
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 Speakers to the default page list on the sidebar #187
Conversation
Reviewer's Guide by SourceryThis pull request introduces new UI translations for Arabic, French, Spanish, Ukrainian, and Russian. It also adds a pop-up for first-time users to select their preferred language, which is then stored in local storage. Additionally, existing translations have been updated for improved accuracy. The sidebar now includes a link to the speakers' page. File-Level Changes
Tips
|
Hi @mariobehling, this PR depend on this PR: #187 |
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 @odkhang - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
webapp/src/locales/es.json
Outdated
@@ -0,0 +1,430 @@ | |||
{ |
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.
suggestion (performance): Consider lazy-loading translations to improve initial load times
With the addition of large JSON files for translations, consider implementing a lazy-loading mechanism for translations. This could help improve initial load times by only loading the necessary language data when required.
{ | |
{ | |
"App:disconnected-warning:text": () => import('./translations/ar.json').then(module => module.default["App:disconnected-warning:text"]), | |
"App:error-code:1006": () => import('./translations/ar.json').then(module => module.default["App:error-code:1006"]), |
webapp/src/locales/ru.json
Outdated
@@ -0,0 +1,430 @@ | |||
{ |
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.
suggestion: Consider having native speakers review the Russian translations for accuracy and cultural appropriateness.
Machine translations or direct copies from other languages may not capture nuances or cultural context. A review by native Russian speakers could significantly improve the quality of the localization.
{ | |
{ | |
"App:disconnected-warning:text": "Соединение потеряно! Пытаемся переподключиться…", | |
"App:error-code:1006": "Не удалось подключиться. Мы попробуем снова, но если проблема повторится, возможно, соединение блокируется брандмауэром вашей сети или VPN на вашем устройстве.", |
webapp/src/locales/ru.json
Outdated
"LandingPage:sessions:featured:link": "к полному расписанию", | ||
"LandingPage:sessions:next:header": "Сессии скоро начнутся", | ||
"LandingPage:sessions:next:link": "к полному расписанию", | ||
"LandingPage:speakers:header": "Наши {{speakers}} спикеры", |
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.
suggestion: Implement a more robust pluralization system to handle language-specific plural rules.
The current string interpolation may not handle plurals correctly for all languages. Consider using a pluralization library or implementing a system that can handle the complexities of different languages' plural rules.
"LandingPage:speakers:header": "Наши {{speakers}} спикеры", | |
"LandingPage:speakers:header": { | |
"one": "Наш спикер", | |
"few": "Наши {{count}} спикера", | |
"many": "Наши {{count}} спикеров", | |
"other": "Наши {{count}} спикеры" | |
} |
Please resolve conflicts. |
This PR closes/references issue #179 . It does so by:
How has this been tested?
Checklist
Summary by Sourcery
This pull request introduces a new feature by adding a default speakers page to the sidebar when the eventyay talk is connected. Additionally, it enhances the application by updating translation files for multiple languages, including Arabic, French, Spanish, Ukrainian, and Russian.