-
Notifications
You must be signed in to change notification settings - Fork 490
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
fix: ko language falls back to ko-KR #2102
Conversation
english has 8 locale files: `ls -lhatr public/locales/en/*.json | wc -l # 8` before this change, ko-KR only had 4 `ls -lhatr public/locales/ko-KR/*.json | wc -l # 4` so I copied them over using `cp -n public/locales/en/*.json public/locales/ko-KR/` after this change `ls -lhatr public/locales/ko-KR/*.json | wc -l # 8`
Sends only a single request for lang via i18n-http-backend see i18next/i18next-http-backend#61
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.
@SgtPooki is this ready for review? I got assigned, but I am not sure as the title says WIP and the PR is draft.
Yea, sorry. it's mostly ready, but I still need to confirm that the language is loading properly during runtime. The unit test seems to indicate that it's working fine. |
@whizzzkid ready now. sorry for the extra noise |
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.
self review
@@ -0,0 +1,106 @@ | |||
{ | |||
"actions": { | |||
"add": "추가하다", |
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 was the most common translation file, but one didn't exist for ko-KR yet. The only modification from en/app.json
is that I added translation for actions.add based off of google translate.
public/locales/ko-KR/notify.json
Outdated
@@ -0,0 +1,30 @@ | |||
{ |
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.
removing. transifex already shows that these strings needs translated: https://www.transifex.com/ipfs/ipfs-webui/translate/#ko_KR/notify-json
public/locales/ko-KR/settings.json
Outdated
@@ -0,0 +1,184 @@ | |||
{ |
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.
removing. transifex already shows that these strings needs translated: https://www.transifex.com/ipfs/ipfs-webui/translate/#ko_KR/settings-json
public/locales/ko-KR/welcome.json
Outdated
@@ -0,0 +1,38 @@ | |||
{ |
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.
removing. transifex already shows that these strings needs translated: https://www.transifex.com/ipfs/ipfs-webui/translate/#ko_KR/welcome-json
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.
self review v2
<div className='pr4 flex items-center lh-copy charcoal f5 fw5 e2e-languageSelector-current' style={{ height: 40 }}> | ||
{getCurrentLanguage()} | ||
</div> | ||
<Button className="tc" bg='bg-teal' minWidth={100} onClick={this.onLanguageEditOpen}> | ||
<Button className="tc e2e-languageSelector-changeBtn" bg='bg-teal' minWidth={100} onClick={this.onLanguageEditOpen}> | ||
{t('app:actions.change')} | ||
</Button> | ||
</div> | ||
|
||
<Overlay show={this.state.isLanguageModalOpen} onLeave={this.onLanguageEditClose} > | ||
<LanguageModal className='outline-0' onLeave={this.onLanguageEditClose} t={t} /> | ||
<LanguageModal className='outline-0 e2e-languageModal' onLeave={this.onLanguageEditClose} t={t} /> |
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.
adding selectors for e2e tests
@@ -25,7 +25,7 @@ const LanguageModal = ({ t, tReady, onLeave, link, className, isIpfsDesktop, doD | |||
{ localesList.map((lang) => | |||
<button | |||
key={`lang-${lang.locale}`} | |||
className='pa2 w-33 flex nowrap bg-transparent bn outline-0 blue justify-center' | |||
className={`pa2 w-33 flex nowrap bg-transparent bn outline-0 blue justify-center e2e-languageModal-lang e2e-languageModal-lang_${lang.locale}`} |
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.
selectors for e2e tests
@@ -16,6 +17,7 @@ i18n | |||
.use(Backend) | |||
.use(LanguageDetector) | |||
.init({ | |||
load: 'currentOnly', // see https://github.com/i18next/i18next-http-backend/issues/61 |
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 is supposed to prevent the selection of multiple languages, but didn't, because i18n-http-backend doesn't know whether the code is valid until it goes to loadPath. The changes to loadPath
below are where the real fix is.
await i18n.init({ | ||
backend: { | ||
...i18n.options?.backend, | ||
backendOptions: [ | ||
i18n.options?.backend?.backendOptions?.[0], | ||
{ | ||
loadPath: `http://localhost:${backendListenerPort}/locales/{{lng}}/{{ns}}.json` | ||
} | ||
] | ||
} | ||
}) |
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.
force requests to our httpServer for i18n-http-backend requests.
const langResult = await i18n.t('app:actions.add', { lng: lang }) | ||
expect(langResult).not.toBe('actions.add') |
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.
validate that i18n key is not returned, which means it's a recognized locale and key
const fallbackResult = fallbackLanguages[prop] | ||
if (fallbackResult != null) { | ||
return fallbackResult[0] | ||
} |
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.
but using fallbacks should be more common
it('should have a languages.json entry for each folder', function () { | ||
const namedLocales = localesList.map(({ locale }) => locale) | ||
expect(namedLocales).toEqual(allLanguages) | ||
}) |
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 ensure our language selector is always up to date. i.e. this test fails if we haven't followed the steps from /docs/LOCALIZATION.md
in the Pulling translations from Transifex
section. specifically, the npx -q @olizilla/lol public/locales > src/lib/languages.json
command.
it('returns unknown when given non-truthy input', () => { | ||
expect(getLanguage()).toBe('Unknown') | ||
expect(getLanguage('')).toBe('Unknown') | ||
expect(getLanguage(null)).toBe('Unknown') | ||
expect(getLanguage(undefined)).toBe('Unknown') | ||
}) |
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.
i don't know if we need this, but I didn't want to change it without knowing for sure.
let languages | ||
const getLanguages = async () => { | ||
if (languages != null) return languages | ||
languages = JSON.parse(await languageFilePromise) | ||
return languages | ||
} |
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.
playwright didn't like me importing json and i didn't want to fiddle with the build since this PR is big enough.
if (lang === 'en') { | ||
// english is the fallback language and we can't guarantee the request wasn't already made, so we resolve for 'en' on any request | ||
|
||
return true | ||
} |
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.
test hangs without this when looping for en
lang
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.
🚀
## [3.0.0](v2.22.0...v3.0.0) (2023-04-24) CID `bafybeic4gops3d3lyrisqku37uio33nvt6fqxvkxihrwlqsuvf76yln4fm` --- ### ⚠ BREAKING CHANGES * migrate to ESM (#2092) ### Features * ipfs-http-client -> kubo-rpc-client ([#2098](#2098)) ([5e53c79](5e53c79)), closes [#issuecomment-1426219633](https://github.com/ipfs/ipfs-webui/issues/issuecomment-1426219633) [/github.com//issues/2079#issuecomment-1426337490](https://github.com/ipfs//github.com/ipfs/ipfs-webui/issues/2079/issues/issuecomment-1426337490) * migrate to ESM ([#2092](#2092)) ([58a737c](58a737c)) ### Bug Fixes * e2e/explore.test.js succeeds in offline mode ([#2109](#2109)) ([a5e9ac6](a5e9ac6)) * ko language falls back to ko-KR ([#2102](#2102)) ([3369800](3369800)) * semantic release custom notes import ([#2113](#2113)) ([2f9f306](2f9f306)) ### Trivial Changes * add NetworkTraffic.stories.js ([#2094](#2094)) ([7a3bf46](7a3bf46)) * pull new translations ([#2101](#2101)) ([cbabac3](cbabac3)) * pull new translations ([#2104](#2104)) ([4a691a2](4a691a2)) * Pull transifex translations ([#2088](#2088)) ([a5b8a1c](a5b8a1c)) * Pull transifex translations ([#2091](#2091)) ([d209863](d209863)) * Pull transifex translations ([#2099](#2099)) ([1cf2fe7](1cf2fe7)) * Pull transifex translations ([#2111](#2111)) ([57d9b63](57d9b63))
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR adds tests for i18n, raw, without react.
It tests that the provided fallbacks are valid, and that they work properly.
Proof of the failing test at https://github.com/ipfs/ipfs-webui/actions/runs/4248889633/jobs/7388520133#step:7:351
fix #2097
Note: I needed to add a temporary translation for app.json's
actions.add
in order to ensure the test passed. lmk if someone has a better idea for how to confirm fallback, because this currently has an implicit dependency on translations between files being different, and we can't necessarily depend on that