DMC/studentportalen
1
1
Fork 0
Code Issues 24 Pull Requests 3 Actions Projects 1 Wiki Activity

Add i18n support #5

Closed
stne3960 wants to merge 13 commits from basic-scaffolding-frontend-lingui into basic-scaffolding-frontend
pull from: basic-scaffolding-frontend-lingui
merge into: DMC:basic-scaffolding-frontend
Conversation 6 Commits 13 Files Changed 19 +3524 -424
stne3960 commented 2025-04-09 11:56:00 +02:00
Owner
Copy Link

This PR adds i18n support with lingui

Once you've added a string, run npm run extractto extract the strings, translate them in ./i18n/locales/sv/messages.po. If running vite, the messages should be compiled automatically, otherwise run npm run compile.

This PR adds i18n support with [lingui](https://lingui.dev/) Once you've added a string, run `npm run extract`to extract the strings, translate them in `./i18n/locales/sv/messages.po`. If running vite, the messages should be compiled automatically, otherwise run `npm run compile`.
stne3960 added 5 commits 2025-04-09 11:56:01 +02:00
ansv7779 requested changes 2025-04-09 12:21:46 +02:00
ansv7779 left a comment
Owner
Copy Link

Overall it looks good, some minor questions in individual comments.

One thing I'm also wondering about is moving the i18n stuff to App.tsx instead of main.tsx. In my mind the App component is responsible for loading all the context required for the application itself, such as the profile, i18n, preferences, or anything else. What is your view on main.tsx vs App.tsx?

Overall it looks good, some minor questions in individual comments. One thing I'm also wondering about is moving the i18n stuff to App.tsx instead of main.tsx. In my mind the App component is responsible for loading all the context required for the application itself, such as the profile, i18n, preferences, or anything else. What is your view on main.tsx vs App.tsx?
frontend/eslint.config.js
@ -7,3 +7,3 @@
export default tseslint.config(
{ ignores: ["dist"] },
{ ignores: ["dist", "src/i18n/locales/**/*.ts"] },
ansv7779 commented 2025-04-09 12:10:47 +02:00
Owner
Copy Link

Do we need this since the generated .ts files have /*eslint-disable*/ at the start?

Do we need this since the generated `.ts` files have `/*eslint-disable*/` at the start?
stne3960 commented 2025-04-09 12:31:57 +02:00
Author
Owner
Copy Link

Otherwise you get the warning Unused eslint-disable directive (no problems were reported) when runnning npm run lint

Otherwise you get the warning `Unused eslint-disable directive (no problems were reported)` when runnning `npm run lint`
ansv7779 marked this conversation as resolved
frontend/package.json Outdated
@ -13,0 +12,4 @@
"preview": "vite preview",
"extract": "lingui extract",
"extract-clean": "lingui extract --clean",
"compile": "lingui compile --typescript"
ansv7779 commented 2025-04-09 12:11:51 +02:00
Owner
Copy Link

Need instructions in the readme what these commands do and what the expected workflow is when adding/translating new content.

Need instructions in the readme what these commands do and what the expected workflow is when adding/translating new content.
👍 1
stne3960 marked this conversation as resolved
frontend/src/i18n/getDefaultLocale.ts Outdated
@ -0,0 +1,9 @@
const supportedLocales = ["sv", "en"];
export function getDefaultLocale(): string {
const lang = navigator.language.split("-")[0]; // "en-US" -> "en"
ansv7779 commented 2025-04-09 12:16:56 +02:00
Owner
Copy Link

Should maybe iterate over navigator.languages instead to find the most preferred locale? If a user sends "Accept-Language: de-DE, en-US, sv-SE" they will get Swedish instead of English since German isn't available.

Also should we maybe default to English if no available language is found instead of Swedish?

Should maybe iterate over `navigator.languages` instead to find the most preferred locale? If a user sends "Accept-Language: de-DE, en-US, sv-SE" they will get Swedish instead of English since German isn't available. Also should we maybe default to English if no available language is found instead of Swedish?
stne3960 commented 2025-04-09 12:57:54 +02:00
Author
Owner
Copy Link

I agree that we should interate over navigator.languages instead.

Regarding defaulting to english if no language is found, very good question. We are Swedish public agency, most outward facing systems default to Swedish, but on the other hand, if they don't have Swedish as an accepted language, there's probably a high likelyhood that they are non-Swedish speakers. I think there's a strong case here to default to English.

I agree that we should interate over `navigator.languages` instead. Regarding defaulting to english if no language is found, very good question. We are Swedish public agency, most outward facing systems default to Swedish, but on the other hand, if they don't have Swedish as an accepted language, there's probably a high likelyhood that they are non-Swedish speakers. I think there's a strong case here to default to English.
ansv7779 commented 2025-04-09 13:08:32 +02:00
Owner
Copy Link

Based on that I'd say defaulting to English makes sense and adhering to the "Accept-Language" header is just nice for users.

- https://www.su.se defaults to Swedish no matter what even if sending "Accept-Language: en-US". - https://scipro.dsv.su.se is only available in English. - https://daisy.dsv.su.se defaults to English and adheres to the "Accept-Language" header. - https://nextilearn.dsv.su.se defaults to English and seems to ignore the "Accept-Language" header. Based on that I'd say defaulting to English makes sense and adhering to the "Accept-Language" header is just nice for users.
stne3960 marked this conversation as resolved
stne3960 commented 2025-04-09 12:48:42 +02:00
Author
Owner
Copy Link

One thing I'm also wondering about is moving the i18n stuff to App.tsx instead of main.tsx. In my mind the App component is responsible for loading all the context required for the application itself, such as the profile, i18n, preferences, or anything else. What is your view on main.tsx vs App.tsx?

I put the i18n initialization stuff in main.tsx because we need to have it in place before we have a profile and display the splash screen, I thought it felt like a natural flow of things, separating bootstrapping the app with rendering and default locale into main.tsx. But I don't have a strong opinion on it. Your approach would add additional logic to App.tsx which may be fine.

> One thing I'm also wondering about is moving the i18n stuff to App.tsx instead of main.tsx. In my mind the App component is responsible for loading all the context required for the application itself, such as the profile, i18n, preferences, or anything else. What is your view on main.tsx vs App.tsx? I put the i18n initialization stuff in main.tsx because we need to have it in place before we have a profile and display the splash screen, I thought it felt like a natural flow of things, separating bootstrapping the app with rendering and default locale into main.tsx. But I don't have a strong opinion on it. Your approach would add additional logic to App.tsx which may be fine.
ansv7779 commented 2025-04-09 12:59:12 +02:00
Owner
Copy Link

I put the i18n initialization stuff in main.tsx because we need to have it in place before we have a profile and display the splash screen, I thought it felt like a natural flow of things, separating bootstrapping the app with rendering and default locale into main.tsx. But I don't have a strong opinion on it. Your approach would add additional logic to App.tsx which may be fine.

I just have some vision in my head of App.tsx looking like

return (
  <ProviderA>
    <ProviderB>
      <ProviderC>
        <ProviderD>
          <Studentportalen />
        </ProviderD>
      </ProviderC>
    </ProviderB>
  </ProviderA>
);

But maybe that adds way too much logic above it to accomplish everything. Eh it's fine, we can always refactor later once we know more.

One final question, is it the t(...) function one should use to show some feedback in a click handler/form submit scenario?

> I put the i18n initialization stuff in main.tsx because we need to have it in place before we have a profile and display the splash screen, I thought it felt like a natural flow of things, separating bootstrapping the app with rendering and default locale into main.tsx. But I don't have a strong opinion on it. Your approach would add additional logic to App.tsx which may be fine. I just have some vision in my head of App.tsx looking like ```typescript return ( <ProviderA> <ProviderB> <ProviderC> <ProviderD> <Studentportalen /> </ProviderD> </ProviderC> </ProviderB> </ProviderA> ); ``` But maybe that adds way too much logic above it to accomplish everything. Eh it's fine, we can always refactor later once we know more. One final question, is it the `t(...)` function one should use to show some feedback in a click handler/form submit scenario?
stne3960 commented 2025-04-09 13:30:45 +02:00
Author
Owner
Copy Link

One final question, is it the t(...) function one should use to show some feedback in a click handler/form submit scenario?

Yes, that's the idea.

> One final question, is it the `t(...)` function one should use to show some feedback in a click handler/form submit scenario? Yes, that's the idea.
stne3960 added 1 commit 2025-04-09 13:49:03 +02:00
stne3960 added 1 commit 2025-04-09 16:29:34 +02:00
stne3960 added 1 commit 2025-04-09 16:33:20 +02:00
stne3960 added 1 commit 2025-04-09 16:37:25 +02:00
ansv7779 commented 2025-04-09 16:45:35 +02:00
Owner
Copy Link

Running npm run build produces no output.

Running `npm run build` produces no output.
stne3960 added 1 commit 2025-04-09 18:31:37 +02:00
stne3960 added 1 commit 2025-04-09 19:22:33 +02:00 
This reverts commit 2faa14dc460a366c537645aa05895e084bca044a.
stne3960 added 1 commit 2025-04-09 19:26:42 +02:00
stne3960 added 1 commit 2025-04-10 10:48:51 +02:00
stne3960 commented 2025-04-10 16:36:30 +02:00
Author
Owner
Copy Link

After discussions, we decided to more forward with something other than lingui

After discussions, we decided to more forward with something other than lingui
stne3960 closed this pull request 2025-04-10 16:36:30 +02:00
stne3960 deleted branch basic-scaffolding-frontend-lingui 2025-04-14 13:46:14 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
ansv7779
Labels
Clear labels
Code Review
Design Review
Design system
Front end
bug
Something is not working
duplicate
This issue or pull request already exists
enhancement
New feature
help wanted
Need some help
invalid
Something is wrong
question
More information is needed
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DMC/studentportalen#5
No description provided.
Delete Branch "basic-scaffolding-frontend-lingui"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?