Skip to content
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

Merged
merged 15 commits into from
Feb 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@
"@svgr/cli": "^5.4.0",
"@types/esm": "^3.2.0",
"@types/jest": "^29.4.0",
"@types/node": "^14.0.27",
"@types/node": "^14.18.36",
"@types/path-browserify": "^1.0.0",
"@typescript-eslint/eslint-plugin": "^5.30.7",
"@typescript-eslint/parser": "^5.30.7",
Expand Down
106 changes: 106 additions & 0 deletions public/locales/ko-KR/app.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
{
"actions": {
"add": "추가하다",
Copy link
Member Author

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.

"apply": "Apply",
"browse": "Browse",
"cancel": "Cancel",
"change": "Change",
"clear": "Clear",
"close": "Close",
"copy": "Copy",
"create": "Create",
"remove": "Remove",
"download": "Download",
"edit": "Edit",
"import": "Import",
"inspect": "Inspect",
"more": "More",
"moreInfo": "More info",
"noThanks": "No thanks",
"ok": "OK",
"pinVerb": "Pin",
"rename": "Rename",
"reset": "Reset",
"save": "Save",
"saving": "Saving…",
"selectAll": "Select all",
"setPinning": "Set pinning",
"submit": "Submit",
"unpin": "Unpin",
"unselectAll": "Unselect all",
"generate": "Generate",
"publish": "Publish",
"downloadCar": "Download as CAR",
"done": "Done"
},
"cliModal": {
"description": "Paste the following into your terminal to do this task in IPFS via the command line. Remember that you'll need to replace placeholders with your specific parameters."
},
"nav": {
"bugsLink": "Report a bug",
"codeLink": "See the code"
},
"status": {
"connectedToIpfs": "Connected to IPFS",
"connectingToIpfs": "Connecting to IPFS…",
"couldNotConnect": "Could not connect to the IPFS API"
},
"apiAddressForm": {
"placeholder": "Enter a URL (http://127.0.0.1:5001) or a Multiaddr (/ip4/127.0.0.1/tcp/5001)"
},
"publicGatewayForm": {
"placeholder": "Enter a URL (https://dweb.link)"
},
"terms": {
"address": "Address",
"addresses": "Addresses",
"advanced": "Advanced",
"agent": "Agent",
"api": "API",
"apiAddress": "API address",
"blocks": "Blocks",
"connection": "Connection",
"downSpeed": "Incoming",
"example": "Example:",
"file": "File",
"files": "Files",
"folder": "Folder",
"folders": "Folders",
"gateway": "Gateway",
"in": "In",
"latency": "Latency",
"loading": "Loading",
"location": "Location",
"name": "Name",
"node": "Node",
"out": "Out",
"peer": "Peer",
"peerId": "Peer ID",
"id": "ID",
"peers": "Peers",
"pinNoun": "Pin",
"pins": "Pins",
"pinStatus": "Pin Status",
"publicKey": "Public key",
"publicGateway": "Public Gateway",
"rateIn": "Rate in",
"rateOut": "Rate out",
"repo": "Repo",
"size": "Size",
"totalIn": "Total in",
"totalOut": "Total out",
"unknown": "Unknown",
"ui": "UI",
"upSpeed": "Outgoing",
"revision": "Revision"
},
"tour": {
"back": "Back",
"close": "Close",
"finish": "Finish",
"next": "Next",
"skip": "Skip",
"tooltip": "Click this button any time for a guided tour on the current page."
},
"startTourHelper": "Start tour"
}
6 changes: 3 additions & 3 deletions src/components/language-selector/LanguageSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,16 @@ class LanguageSelector extends Component {
return (
<Fragment>
<div className='flex'>
<div className='pr4 flex items-center lh-copy charcoal f5 fw5' style={{ height: 40 }}>
<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} />
Comment on lines +22 to +31
Copy link
Member Author

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

</Overlay>
</Fragment>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}`}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectors for e2e tests

onClick={() => handleClick(lang.locale)}>
{ lang.nativeName }
</button>
Expand Down
9 changes: 7 additions & 2 deletions src/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import LanguageDetector from 'i18next-browser-languagedetector'
import pkgJson from '../package.json'

import locales from './lib/languages.json'
import getValidLocaleCode from './lib/i18n-localeParser.js'

const { version } = pkgJson
export const localesList = Object.values(locales)
Expand All @@ -16,6 +17,7 @@ i18n
.use(Backend)
.use(LanguageDetector)
.init({
load: 'currentOnly', // see https://github.com/i18next/i18next-http-backend/issues/61
Copy link
Member Author

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.

backend: {
backends: [
LocalStorageBackend,
Expand All @@ -27,8 +29,11 @@ i18n
expirationTime: (!process.env.NODE_ENV || process.env.NODE_ENV === 'development') ? 1 : 7 * 24 * 60 * 60 * 1000
},
{ // HttpBackend
// ensure a relative path is used to look up the locales, so it works when loaded from /ipfs/<cid>
loadPath: 'locales/{{lng}}/{{ns}}.json'
loadPath: (lngs, namespaces) => {
const locale = getValidLocaleCode({ i18n, localeCode: lngs[0], languages: locales })
// ensure a relative path is used to look up the locales, so it works when loaded from /ipfs/<cid>
return `locales/${locale}/${namespaces}.json`
}
}
]
},
Expand Down
92 changes: 92 additions & 0 deletions src/i18n.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/* global describe, it, expect, beforeAll, afterAll */
// @ts-check
import { createServer } from 'http-server'
import i18n, { localesList } from './i18n.js'
import getPort from 'get-port'

const backendListenerPort = await getPort({ port: getPort.makeRange(3000, 4000) })

const allLanguages = localesList.map(({ locale }) => locale)

/**
* @type {import('http-server').HTTPServer}
*/
let httpServer
beforeAll(async function () {
httpServer = createServer({
root: './public',
cors: true
})
await httpServer.listen(backendListenerPort)

// initialize i18n
await i18n.init({
backend: {
...i18n.options?.backend,
backendOptions: [
i18n.options?.backend?.backendOptions?.[0],
{
loadPath: `http://localhost:${backendListenerPort}/locales/{{lng}}/{{ns}}.json`
}
]
}
})
Comment on lines +23 to +33
Copy link
Member Author

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.

})

afterAll(async function () {
await httpServer.close()
})
describe('i18n', function () {
it('should have a default language', function () {
expect(i18n.language).toBe('en-US')
expect(i18n.isInitialized).toBe(true)
})

it('should return key for non-existent language', function () {
expect(i18n.t('app:actions.add', { lng: 'xx' })).toBe('actions.add')
})

allLanguages.concat('ko').forEach((lang) => {
describe(`lang=${lang}`, function () {
it(`should be able to switch to ${lang}`, async function () {
await i18n.changeLanguage(lang)

expect(i18n.language).toBe(lang)
})

it(`should have a key for ${lang}`, async function () {
// key and namespace that don't exist return the key without the leading namespace
expect(await i18n.t('someNs:that.doesnt.exist', { lng: lang })).toBe('that.doesnt.exist')
// missing key on existing namespace returns that key
expect(await i18n.t('app:that.doesnt.exist', { lng: lang })).toBe('that.doesnt.exist')
const langResult = await i18n.t('app:actions.add', { lng: lang })
expect(langResult).not.toBe('actions.add')
Comment on lines +62 to +63
Copy link
Member Author

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

})
})
})

describe('fallback languages', function () {
/**
* @type {import('i18next').FallbackLngObjList}
*/
const fallbackLanguages = /** @type {import('i18next').FallbackLngObjList} */(i18n.options.fallbackLng)
for (const lng in fallbackLanguages) {
if (lng === 'default') {
continue
}
const fallbackArr = fallbackLanguages[lng]
fallbackArr.forEach((fallbackLang) => {
it(`fallback '${fallbackLang}' (for '${lng}') is valid`, async function () {
expect(allLanguages).toContain(fallbackLang)
})
Comment on lines +79 to +81
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure that any configured fallback languages are correct.

})
it(`language ${lng} should fallback to ${fallbackArr[0]}`, async function () {
const result = await i18n.t('app:actions.add', { lng })
const englishResult = await i18n.t('app:actions.add', { lng: 'en' })
const fallbackResult = await i18n.t('app:actions.add', { lng: fallbackArr[0] })
expect(result).toBe(fallbackResult)
expect(result).not.toBe(englishResult)
Comment on lines +84 to +88
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only way I could think of to test that it was translating to it's fallback language properly, but it's still not actually confirming that it's translating to the expected language. It only confirms that it's translated to a fallback that is not english. We could do some intelligent parsing of the language files themselves, but I thought this was good enough. i'm open to other ideas.

})
}
})
})
46 changes: 46 additions & 0 deletions src/lib/i18n-localeParser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
*
* @param {object} options
* @param {import('i18next').i18n} options.i18n
* @param {string} options.localeCode
* @param {Record<string, { locale: string, nativeName: string, englishName: string }>} options.languages
*
* @returns {string}
*/
export default function getValidLocaleCode ({ i18n, localeCode, languages }) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before creating this, I created src/lib/i18n.test.js. Then I created this file, added tests for it, and made sure the existing src/lib/i18n.test.js tests were passing.

const info = languages[localeCode]

if (info != null) {
return localeCode
}

const fallbackLanguages = i18n.options.fallbackLng[localeCode]
if (info == null && fallbackLanguages != null) {
/**
* check fallback languages before attempting to split a 'lang-COUNTRY' code
* fixed issue with displaying 'English' when i18nLng is set to 'ko'
* discovered when looking into https://github.com/ipfs/ipfs-webui/issues/2097
*/
const fallback = fallbackLanguages
for (const locale of fallback) {
const fallbackInfo = languages[locale]

if (fallbackInfo != null) {
return fallbackInfo.locale
}
}
}

// if we haven't got the info in the `languages.json` we split it to get the language
const langOnly = localeCode.split('-')[0]
if (languages[langOnly]) {
return langOnly
}
// if the provided localeCode doesn't have country, but we have a supported language for a specific country, we return that
const langWithCountry = Object.keys(languages).find((key) => key.startsWith(localeCode))
if (langWithCountry) {
return langWithCountry
}
Comment on lines +39 to +43
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have to do this, as that's kind of what fallbacks are for.. but this also helps us catch unexpected locale codes automatically instead of needing to update fallbackLang for everything.

We may still need to update fallbackLang's to solve issues where ar-SA would rather resolve to ar-QA than ar or similar.


return 'en'
}
Loading