search

ajnart / homarr

Customizable browser's home page to interact with your homeserver's Docker containers (e.g. Sonarr/Radarr)
https://homarr.dev
MIT License
5.52k stars 255 forks source link

fix: #1859 dayjs locale translation #1868

Closed manuel-rw closed 5 months ago

manuel-rw commented 5 months ago

Fixes #1859

github-actions[bot] commented 5 months ago

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 16.54% 4726 / 28562
🔵 Statements 16.54% 4726 / 28562
🔵 Functions 6.5% 29 / 446
🔵 Branches 37.8% 124 / 328
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/pages/_app.tsx 0% 0% 0% 0% 1-204
src/tools/language.ts 100% 100% 100% 100%
Generated in workflow #6273
ajnart commented 5 months ago

You could keep my comparaison with CR for crowdin and make the dayjslocale optional so that we can only add the field when it's needed. Otherwise it adds another complexity to the file. But that's okay

I mean apply the locale or the dayjslocale if it exists

SeDemal commented 5 months ago

Locale was added as days locale already, no need to double it juste make that one optional and remove it from the crowding entry

manuel-rw commented 5 months ago

IMO locale could be different from the dayjs locale used. There are a few discrepancies. Using a dedicated variable also makes it clear what it's used for.

ajnart commented 5 months ago

IMO locale could be different from the dayjs locale used. There are a few discrepancies. Using a dedicated variable also makes it clear what it's used for.

Yeah that's my point, but it doesn't need to be there if there are no differences

manuel-rw commented 5 months ago

IMO locale could be different from the dayjs locale used. There are a few discrepancies. Using a dedicated variable also makes it clear what it's used for.

Yeah that's my point, but it doesn't need to be there if there are no differences

I disagree, the locale for Crowdin is cr and we can simply not set the property when it's not supported by dayjs.

ajnart commented 5 months ago

IMO locale could be different from the dayjs locale used. There are a few discrepancies. Using a dedicated variable also makes it clear what it's used for.

Yeah that's my point, but it doesn't need to be there if there are no differences

I disagree, the locale for Crowdin is cr and we can simply not set the property when it's not supported by dayjs.

yeah that's what I meant

Spillebulle commented 5 months ago

Works perfectly now. Nice