added react-i18next to FE #3682
Conversation
| @@ -57,6 +52,7 @@ export const WorkspaceScheduleButton: React.FC<WorkspaceScheduleButtonProps> = ( | |||
| deadlineMinusEnabled, | |||
| canUpdateWorkspace, | |||
| }) => { | |||
| const { t } = useTranslation("workspacePage") | |||
There was a problem hiding this comment.
This is the primary way we should access translations in components: by using the useTranslation hook. Here, I have passed the translation hook the namespace I am interested in procuring translations from.
| @@ -34,70 +21,72 @@ export const getStatus = ( | |||
| icon: React.ReactNode | |||
| } => { | |||
| const status = getWorkspaceStatus(build) | |||
| const { t } = i18next | |||
There was a problem hiding this comment.
Here is another way we can access translations: directly from i18next (please note import above). I am accessing translations this way because I am working within a function, not a component (and you can't use hooks in functions).
There was a problem hiding this comment.
Why is the hook preferable when you have the choice?
There was a problem hiding this comment.
Oh is it because it takes the namespace?
There was a problem hiding this comment.
Definitely! But also, useTranslation gets called when language changes (i18next.changeLanguage). Importing directly means translations are not updated when language changes. There are workarounds here, and we don't need to worry about it right now because we're not actually translating our app, but it's one thing less to worry down the road if we ever do translate.
There was a problem hiding this comment.
Great to know. Can you add something to Notion about this?
| switch (status) { | ||
| case undefined: | ||
| return { | ||
| text: StatusLanguage.loading, | ||
| text: t("common:workspace_status.loading", { ns: "common" }), |
There was a problem hiding this comment.
Here's another way to namespace. There's actually a third way to namespace: I could write
t("common:workspace_status.loading"), but some people find this more difficult to read. I don't have a preference myself.
| @@ -171,7 +174,7 @@ describe("WorkspacePage", () => { | |||
| await testStatus(MockDeletingWorkspace, DisplayStatusLanguage.deleting) | |||
| }) | |||
| it("shows the Deleted status when the workspace is deleted", async () => { | |||
| await testStatus(MockDeletedWorkspace, DisplayStatusLanguage.deleted) | |||
| await testStatus(MockDeletedWorkspace, t("workspace_status.deleted", { ns: "common" })) | |||
There was a problem hiding this comment.
We should use our new translations in our tests, too, so our tests don't become brittle.
| <Route path={path ?? route} element={<RequireAuth>{ui}</RequireAuth>} /> | ||
| </Routes> | ||
| </ThemeProvider> | ||
| </I18nextProvider> |
There was a problem hiding this comment.
When testing, we must wrap rendered components inside this provider.
|
@code-asher Yeah, I generally like a |
site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
Outdated
Show resolved
Hide resolved
| @@ -3,6 +3,8 @@ import { createRoot } from "react-dom/client" | |||
| import { Interpreter } from "xstate" | |||
| import { App } from "./app" | |||
|
|
|||
| import "./i18n" | |||
There was a problem hiding this comment.
❓ Why do we import this here? Is that to initialize translations or something?
There was a problem hiding this comment.
Yup, it's part of the setup!
site/src/i18n/i18n.ts
Outdated
| resources, | ||
| }) | ||
| .catch((error) => { | ||
| console.error(error) |
There was a problem hiding this comment.
❓ What will error look like? Would prefixing the error messages with something like [Translation Service] help us distinguish these vs. others?
There was a problem hiding this comment.
I will add that! But really, I added this catch block to get around a linting error: no-floating-promises
It's not really adding a lot beside quieting lint AFAIK.
There was a problem hiding this comment.
Oh we are going to add that to code-server! I think it can be super helpful in insuring you don't accidentally forget to await a promise. Example: ideally it would catch issues like this one
| @@ -0,0 +1 @@ | |||
| export * from "./i18n" | |||
There was a problem hiding this comment.
Yeah, I figure this is a good place for one.
resolves #3486
We now have FE translations, which will make handling strings a lot simpler and less error prone. This PR encapsulates the addition of the library, as well as a proof of concept so folks can get rolling quickly.
Translations and translation config is all located in a new directory:
src/i18n. Translations can be created and then used in files and tests. I've left some comments below to illustrate usage.After this PR is merged up, we should cease to use
Languageobjects, and instead should add translation strings for all of our text. Translation strings can be broken out into different namespaces. Namespaces are located insrc/i18n/en. After adding a new namespace, be sure to export that namespace module insrc/i18n/en/index.ts. Over time, we should try to remove old Language blocks as we come across them in areas we're working on.Let me know if you have any questions or concerns!