-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Improve i18n support and add missing translations #6070
base: main
Are you sure you want to change the base?
Conversation
- Add missing translations for all supported languages - Add i18n test utilities and translation tests - Fix duplicate key detection in tests - Update UI components to use translations
- Add missing translation keys for browser, user avatar, and suggestions - Add translations for all supported languages - Create test to catch hardcoded English strings - Fix components to use translation system properly
- Remove CHAT$WHAT_DO_YOU_WANT_TO_BUILD and LANDING$BUILD_PROMPT in favor of SUGGESTIONS$WHAT_TO_BUILD - Remove unused keys: - STATUS$CONNECTED_TO_SERVER - BROWSER$SCREENSHOT and BROWSER$SCREENSHOT_ALT - SUGGESTIONS$AUTO_MERGE and SUGGESTIONS$AUTO_MERGE_PRS - USER$AVATAR_PLACEHOLDER (duplicate)
- Remove WORKSPACE$LABEL (unused) - Remove CONNECT_TO_GITHUB_BY_TOKEN_MODAL$TERMS_OF_SERVICE (unused)
…penHands into add-japanese-translations
@amanape , apologies that this is a huge PR but hopefully the logic is relatively straightforward. |
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.
About time! This is going to cause some open PRs to have to resolve some conflicts 😭 but definitely worth it.
If you could just address / answer the comments first 😃
describe('Translations', () => { | ||
describe('Translation Coverage', () => { |
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.
Nested describe, we can unpack to have under a single describe since there are no other describe suites present.
frontend/src/i18n/index.ts
Outdated
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.
Are these changes necessary? They seem repetitive and it previously worked without them
- Use screen.getByPlaceholderText() instead of direct container query - Add screen import from @testing-library/react - Remove unnecessary files and revert i18n changes
- Remove nested describe block as suggested in review - Keep test functionality unchanged
c68ce85
to
3f51460
Compare
c8f730d
to
59d27a2
Compare
OK @amanape , I think this is ready for review again! |
This PR improves the internationalization (i18n) support in the frontend by:
Adding missing translations for all supported languages (zh-CN, zh-TW, ko-KR, de, no, it, pt, es, ar, fr, tr) for:
Adding i18n test utilities and translation tests:
Updating UI components to use translations consistently
Fixes #6066
Fixes #4280
Here are examples of the localized screens. I have confirmed that all settings menus, etc. are localized.
To run this PR locally, use the following command: