feat: add i18n translation #565
Conversation
|
Awesome Stephan, thanks! I'll review your PR next week. |
|
maybe add an function that allows to set i18n data? |
Great idea, i will add it |
josdejong
left a comment
josdejong
left a comment
There was a problem hiding this comment.
First: awesome job Stephan, this is quite an undertaking. It looks solid and well taken care of. Thanks!
I made quite some inline comments. Most are small, some require some thinking though. Can you have a look at those please?
| {value.length} | ||
| {value.length === 1 ? 'item' : 'items'} | ||
| </Tag> | ||
| {value.length === 1 ? t('item') : t('items')} |
There was a problem hiding this comment.
I think this line should stay inside <Tag>...</Tag>, like it was?
src/lib/i18n/locales.ts
Outdated
| } | ||
| } | ||
|
|
||
| export const russia: Language = { |
There was a problem hiding this comment.
I think russia should be russian
src/lib/i18n/types.ts
Outdated
There was a problem hiding this comment.
Can you remove this empty file i18n/types.ts?
| { | ||
| type: 'button', | ||
| icon: faUndo, | ||
| title: 'Undo (Ctrl+Z)', |
There was a problem hiding this comment.
Can you translate all items of the TableMenu?
| text: 'Cut', | ||
| title: 'Cut selected contents, formatted with indentation (Ctrl+X)', | ||
| text: t('cut'), | ||
| title: t('cutSelectedContentFormattedWithIndentation') + ' (Ctrl+X)', |
There was a problem hiding this comment.
Can you give all title translations in the createTableContextMenuItems a shorter name, like:
cutSelectedContentFormattedWithIndentation->cutFormattedTitlecutSelectedContentWithoutIndent->cutCompactedTitlepastClipboardContent->pasteTitle- etc
src/lib/i18n/index.ts
Outdated
|
|
||
| if (params) { | ||
| for (const param in params) { | ||
| translation = translation.replace(new RegExp(`{{${param}}}`, 'g'), params[param]) |
There was a problem hiding this comment.
Can you use a replaceAll here? In case we use the same param twice.
There was a problem hiding this comment.
@josdejong
replaceAll was introduced only in es2021. Currently, the lib uses es2020, and TypeScript complains about it. What's the best way to fix this behavior? Can I change compileOptions?
There was a problem hiding this comment.
O that is a good point! I think it's best to write a little replaceAll function ourselves instead of using the built-in version.
src/lib/i18n/index.ts
Outdated
| @@ -0,0 +1,42 @@ | |||
| import { getContext, hasContext, setContext } from 'svelte' | |||
There was a problem hiding this comment.
Can you run the tests and update the snapshots that are failing right now?
(this comment is not related to this piece of code)
src/lib/i18n/index.ts
Outdated
| @@ -0,0 +1,42 @@ | |||
| import { getContext, hasContext, setContext } from 'svelte' | |||
| import type { Language, TranslationKey } from '$lib/types' | |||
There was a problem hiding this comment.
Can you write documentation in the README.md about the new language feature?
(this comment is not related to this piece of code)
src/lib/i18n/index.ts
Outdated
|
|
||
| export function t(key: keyof TranslationKey, params?: Record<string, string>): string { | ||
| const ctx = getI18nContext() | ||
| let translation: string = get(ctx.values)?.[key] ?? key |
There was a problem hiding this comment.
I think it would be good to add an extra fallback to the default language, English. When we add new language keys for new features, I think it can easily happen that languages are a step behind, so it may be good to cater for that.
So then it can become:
let translation: string = get(ctx.values)?.[key] ?? defaultLanguage?.[key] ?? keyWhat do you think?
src/lib/i18n/index.ts
Outdated
| } | ||
|
|
||
| export function getI18nContext() { | ||
| return getContext<I18nContext>(I18N_CONTEXT_KEY) |
There was a problem hiding this comment.
When running in a develop environment, I get errors like:
Uncaught Svelte error: lifecycle_outside_component `getContext(...)` can only be used during component initialisation
Did you encounter that too?
|
@strictKraken did you see my feedback from last week? |
Thanks for the detailed review! I was a bit busy last week, so I'll take a closer look at the review and make some fixes this week !! |
|
Thanks! No problem at all, no need to rush or anything. |
|
This PR is excellent, keep it up. |
4 participants
#564
Apologies for the formatting, I couldn't find a contribution guide.
I wasn't sure if it's okay to use third-party libraries for translation, so I added a simple translation implementation.
I don't know Chinese very well, but AI-generated translations could be added if someone can review them.