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

feat: add support for all languages from old homarr #1394

Merged
merged 11 commits into from
Nov 2, 2024

Conversation

Meierschlumpf
Copy link
Member

@Meierschlumpf Meierschlumpf commented Oct 31, 2024


Homarr

Thank you for your contribution. Please ensure that your pull request meets the following pull request:

  • Builds without warnings or errors (pnpm buid, autofix with pnpm format:fix)
  • Pull request targets dev branch
  • Commits follow the conventional commits guideline
  • No shorthand variable names are used (eg. x, y, i or any abbrevation)

All translations from non english were migrated from old homarr with a script. It may contain incorrect translations. Overall 300 / 1050 translations were copied.

Created issue #1402 for crowdin integration and made a prototype with a project on my own to verify it would work with the current structure

@Meierschlumpf Meierschlumpf added the enhancement New feature or request label Oct 31, 2024
@Meierschlumpf Meierschlumpf self-assigned this Oct 31, 2024
@Meierschlumpf Meierschlumpf requested a review from a team as a code owner October 31, 2024 21:13
Copy link

deepsource-io bot commented Oct 31, 2024

Here's the code health analysis summary for commits deb5a71..f7acb63. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ SuccessView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Copy link

github-actions bot commented Oct 31, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 22.2% 6345 / 28570
🔵 Statements 22.2% 6345 / 28570
🔵 Functions 23.96% 243 / 1014
🔵 Branches 60.41% 766 / 1268
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
apps/nextjs/src/app/[locale]/layout.tsx 0% 0% 0% 0% 1-110
apps/nextjs/src/app/[locale]/_client-providers/dayjs-loader.tsx 0% 0% 0% 0% 1-12
apps/nextjs/src/app/[locale]/_client-providers/mantine.tsx 0% 0% 0% 0% 1-77
apps/nextjs/src/app/[locale]/manage/integrations/_components/secrets/integration-secret-card.tsx 0% 0% 0% 0% 1-75
apps/nextjs/src/components/language/language-combobox.tsx 0% 0% 0% 0% 1-94
packages/common/src/hooks.ts 30.76% 100% 0% 30.76% 10-11, 14-23
packages/translation/src/config.ts 63.45% 100% 0% 63.45% 11-14, 16-17, 24-25, 27-28, 35-36, 38-39, 46-47, 49-50, 57-58, 60-61, 68-69, 71-72, 79-80, 82-83, 90-91, 93-94, 101-102, 104-105, 113-114, 116-117, 124-125, 127-128, 135-136, 138-139, 146-147, 149-150, 157-158, 160-161, 168-169, 171-172, 179-180, 182-183, 190-191, 193-194, 201-202, 204-205, 212-213, 215-216, 223-224, 226-227, 234-235, 237-238, 245-246, 248-249, 256-257, 259-260, 267-268, 270-271, 278-279, 281-282, 289-290, 292-293, 300-301, 303-304, 311-314, 316-317, 324-325, 327-328, 335-336, 338-339, 359
packages/translation/src/dayjs.ts 0% 0% 0% 0% 1-48
packages/translation/src/mapping.ts 100% 100% 9.09% 100%
packages/translation/src/type.ts 0% 0% 0% 0%
packages/ui/src/hooks/use-translated-mantine-react-table.ts 0% 0% 0% 0% 1-25
Generated in workflow #3513 for commit f7acb63 by the Vitest Coverage Report Action

Copy link
Contributor

@ajnart ajnart left a comment

Choose a reason for hiding this comment

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

Great work ! It's impressive

  • Did you make sure that this format is compatible with crowdin ? It might require to have a .json file and import it in the .ts as a valid directly (no need for validation)
  • Do you plan to add back the live translation feature (Crowdin) ? I made it so that the script is added to the page depending on the user preference
  • I saw you're switching to next-intl (the translation thing from crowdin), they support a localstorage adapter if I recall correctly so you could make it so that translations are only passed once then saved in localstorage, also could utilize translations come from Crowdin's CDN to synchronize them with the app (in a chron job?) with Crowdin's OTA translations feature. (I believe they gave us a Pro plan for open-source so we have pretty much all features for free)
  • Last comment the i18n ally vscode extension works really well with this plugin so maybe add a config into the .vscode folder that takes care of this, then you can have auto-add keys/inline keys replacement (it will show the translations) and also a warning when a text is not translated

@Meierschlumpf Meierschlumpf marked this pull request as draft November 1, 2024 09:44
@Meierschlumpf
Copy link
Member Author

Great work ! It's impressive

  • Did you make sure that this format is compatible with crowdin ? It might require to have a .json file and import it in the .ts as a valid directly (no need for validation)
  • Do you plan to add back the live translation feature (Crowdin) ? I made it so that the script is added to the page depending on the user preference
  • I saw you're switching to next-intl (the translation thing from crowdin), they support a localstorage adapter if I recall correctly so you could make it so that translations are only passed once then saved in localstorage, also could utilize translations come from Crowdin's CDN to synchronize them with the app (in a chron job?) with Crowdin's OTA translations feature. (I believe they gave us a Pro plan for open-source so we have pretty much all features for free)
  • Last comment the i18n ally vscode extension works really well with this plugin so maybe add a config into the .vscode folder that takes care of this, then you can have auto-add keys/inline keys replacement (it will show the translations) and also a warning when a text is not translated
  1. Yeah TS is not working great, it wants to translate the import statements 😂
    image

  2. Yes it's planned to add this again

  3. Okay nice, I think CDN is a little bit overkill for now, especially as we will release automatically every 7 days, but the locale storage thing could be quite nice

  4. I think you already added it, not sure if it's working when I changed it from next-international to next-intl tough.

@Meierschlumpf
Copy link
Member Author

Test issue: vitest-dev/vitest#5101
Fixing it right now by adding default coverage excludes to vitest.config.ts

@Meierschlumpf Meierschlumpf marked this pull request as ready for review November 2, 2024 17:38
@Meierschlumpf Meierschlumpf requested review from a team and ajnart November 2, 2024 17:38
@manuel-rw
Copy link
Member

Yeah TS is not working great, it wants to translate the import statements 😂

So... what do we do about this then?

Okay nice, I think CDN is a little bit overkill for now, especially as we will release automatically every 7 days, but the locale storage thing could be quite nice

I agree, let's plan this for >1.0.

@Meierschlumpf
Copy link
Member Author

So... what do we do about this then?

I already changed it to JSON

@manuel-rw
Copy link
Member

So... what do we do about this then?

I already changed it to JSON

Stupid question. Would we have an advantage storing as JSON but converting to TS on the fly / on commit?
If not, I'm okay with this solution.

@Meierschlumpf Meierschlumpf merged commit 0ff7c89 into dev Nov 2, 2024
10 checks passed
@Meierschlumpf Meierschlumpf deleted the additional-language-support branch November 2, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants