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

Fix missing i18n keys for FR, IT, EN, DE #1993

Merged
merged 6 commits into from
Oct 18, 2024

Conversation

khwadj
Copy link
Contributor

@khwadj khwadj commented Oct 3, 2024

This PR aims at fixing the missing localization keys throughout the language files.

For the few languages I more or less speak (french, spanish, italian, german), those new keys have also been translated.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

@khwadj khwadj changed the title Fix missing i18n keys with default englis, and fill some of them Fix missing i18n keys with default english, and fill some of them (FR, IT, EN, DE) Oct 3, 2024
@lrljoe
Copy link
Collaborator

lrljoe commented Oct 4, 2024

Will take a look this weekend to get this merged in. Don't worry about the test that failed, it'll pass when I rerun it

@lrljoe
Copy link
Collaborator

lrljoe commented Oct 5, 2024

Okay, having reviewed this PR - please don't add in the English localisations into the non-English locale files.

The reason being - I do contact those who contribute localisations/translations when I spot an unlocalised string, and I won't spot it with this!

It should fail back to English if there's no translated key present.

Please update your PR, so that you're only adding missing keys that are valid localisations.

…and re-ordering a few misplaced ones"

This reverts commit d440bd2.
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.34%. Comparing base (0c8ec45) to head (ee67af6).
Report is 129 commits behind head on development.

Additional details and impacted files
@@                Coverage Diff                @@
##             development    #1993      +/-   ##
=================================================
+ Coverage          87.30%   88.34%   +1.04%     
- Complexity          1672     1855     +183     
=================================================
  Files                150      174      +24     
  Lines               3891     4274     +383     
=================================================
+ Hits                3397     3776     +379     
- Misses               494      498       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@khwadj
Copy link
Contributor Author

khwadj commented Oct 7, 2024

I've updated this PR according to your recommendations.

The fallback language is defined at the app level. In our case, that's french, so missing keys will have an impact, and it will for anybody not defining EN as their fallback language at the app level, since I've found missing keys in all language files.

I'm okay fixing only a few languages if that's what you want, since it's ultimately what serves me.

Note: I'm having an issue with fallback locale not working, but it might be the laravel version we're using (v10.48.12, I plan on ugrading later this month to the latest one), and I don't have much time available at the moment to investigate it, so let's ignore that.

On a related but different subject, some keys would benefits from being reworked as single strings with variables rather that several ones. "Showing X of Y on Z results" is an english sentence and translates very poorly in other languages when maintaining this syntax. Defining a single translation key with X, Y, and Z variables would gracefully solve this issue by allowing each language to phrase it properly. Let me know if you're open to a discussion and/or change relatively to this aspect.

@khwadj khwadj changed the title Fix missing i18n keys with default english, and fill some of them (FR, IT, EN, DE) Fix missing i18n keys for FR, IT, EN, DE Oct 7, 2024
@lrljoe
Copy link
Collaborator

lrljoe commented Oct 18, 2024

On a related but different subject, some keys would benefits from being reworked as single strings with variables rather that several ones. "Showing X of Y on Z results" is an english sentence and translates very poorly in other languages when maintaining this syntax. Defining a single translation key with X, Y, and Z variables would gracefully solve this issue by allowing each language to phrase it properly. Let me know if you're open to a discussion and/or change relatively to this aspect.

Very happy to have a discussion around that. I believe from memory that the translation key for "Showing X of Y on Z results" mirrors the default Livewire/Laravel behaviour, but more than happy to amend.

I am trying to keep track of the contributors for localisations on the Discord, so do poke me if you haven't already.

@lrljoe lrljoe merged commit 9fc6ad8 into rappasoft:development Oct 18, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants