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: Tagline V3: support for countries-only locales: (eg _FR) #5370

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Jun 13, 2024

Hi everyone!

Today, we only support languages and languages+countries codes.
This PR allows supporting countries-only codes (e.g.: _FR)

@g123k g123k added the tagline label Jun 13, 2024
@g123k g123k self-assigned this Jun 13, 2024
@g123k g123k requested a review from a team as a code owner June 13, 2024 10:55
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 7.26%. Comparing base (4d9c7fc) to head (1ca2630).
Report is 200 commits behind head on develop.

Files Patch % Lines
...h_app/lib/data_models/news_feed/newsfeed_json.dart 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #5370      +/-   ##
==========================================
- Coverage     9.54%   7.26%   -2.29%     
==========================================
  Files          325     391      +66     
  Lines        16411   20038    +3627     
==========================================
- Hits          1567    1456     -111     
- Misses       14844   18582    +3738     

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

@monsieurtanuki
Copy link
Contributor

Today, we only support languages and languages+countries codes. This PR allows supporting countries-only codes (e.g.: _FR)

Just curious: is that supposed to happen one day? Doesn't seem to be the case in ios or android for the moment.

@g123k
Copy link
Collaborator Author

g123k commented Jun 13, 2024

Today, we only support languages and languages+countries codes. This PR allows supporting countries-only codes (e.g.: _FR)

Just curious: is that supposed to happen one day? Doesn't seem to be the case in ios or android for the moment.

Both JSON are outdated (and they are also invalid).
For now, the idea is to deploy Nutri-score outside of France (that's one of the reasons 4.15.0 is still not released).
(And we still don't know when it will be…)

@monsieurtanuki
Copy link
Contributor

What is supposed to happen with _BE: which language would be used?
Even if the text is supposed to be different for fr_FR and fr_BE, it will definitely be different for fr_BE and nl_BE.
Would make sense to use the language_country syntax, with language only and/or country only as a fallback.

@g123k
Copy link
Collaborator Author

g123k commented Jun 13, 2024

What is supposed to happen with _BE: which language would be used? Even if the text is supposed to be different for fr_FR and fr_BE, it will definitely be different for fr_BE and nl_BE. Would make sense to use the language_country syntax, with language only and/or country only as a fallback.

That's exactly what it is.

The order of priority is:

  • Strict matching (so test if fr_FR exists)
  • If not, we check if there is something with the language (fr)
  • And then, by the country _FR (the feature of this PR)
  • And if nothing is available -> default value

@monsieurtanuki
Copy link
Contributor

And then, by the country _FR (the feature of this PR)

That's precisely what I don't get: in which cases are we supposed to receive messages with country but no language?
Have you been confronted to that? Are they supposed to send that?

Maybe it's similar to @yarons in #5339: using the app in Israel but in English, not in Hebrew, right?
Then we would be better off listing all the keys and finding the first one that matches .._IL, possibly with a preference for English in general.
Even then, we'll have either RTL or LTR languages, and that would need to be dealt with.

Anyway, I'm surprised we may rely on receiving _cc messages.
What do you think of my .._cc look-up suggestion: would that solve the "unexpected language x country combo" case?

@g123k
Copy link
Collaborator Author

g123k commented Jun 14, 2024

And then, by the country _FR (the feature of this PR)

That's precisely what I don't get: in which cases are we supposed to receive messages with country but no language? Have you been confronted to that? Are they supposed to send that?

Maybe it's similar to @yarons in #5339: using the app in Israel but in English, not in Hebrew, right? Then we would be better off listing all the keys and finding the first one that matches .._IL, possibly with a preference for English in general. Even then, we'll have either RTL or LTR languages, and that would need to be dealt with.

Anyway, I'm surprised we may rely on receiving _cc messages. What do you think of my .._cc look-up suggestion: would that solve the "unexpected language x country combo" case?

The way the JSON is split is that we have on one side translated news and on the other a feed of news.
That's why we can ask for a feed for each country for example, with still translated content.

Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

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

👍

@teolemon
Copy link
Member

Small merge conflict @g123k

@teolemon teolemon merged commit 229ea66 into openfoodfacts:develop Jun 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants