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: new localized label 'sep' as separator before colon (especially in French) #3753

Merged
merged 7 commits into from
Mar 2, 2023

Conversation

monsieurtanuki
Copy link
Contributor

Impacted files:

  • app_en.arb: new label 'sep' as separator before colon; minor consistency fix
  • app_fr.arb: new label 'sep' as separator before colon; minor consistency fixes

What

Part of

@codecov-commenter
Copy link

Codecov Report

Merging #3753 (8b4629e) into develop (90a9bc5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3753   +/-   ##
========================================
  Coverage    10.40%   10.40%           
========================================
  Files          272      272           
  Lines        13835    13835           
========================================
  Hits          1440     1440           
  Misses       12395    12395           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +3 to +6
"sep": "",
"@sep": {
"description": "Separator just before a colon (':'). Probably only populated in French and empty in other languages."
},
Copy link
Member

Choose a reason for hiding this comment

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

I've read the discussion in the other PR but do we really need this, shouldn't these spaces be added in the Strings itself as you did in the app_fr.arb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@M123-dev You're right, we don't need it as long as we have full sentences already localized.

But if we want to build a String with very specific business rules, we need this extra label. Off server needed it. And that would be used in the referenced PR.

Should the referenced PR use a colon rather than a comma, that's another story.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the server side, there are many places where we display a field name, a colon and a value. (e.g. "Categories: Coffees"). We do that with something that looks like [field name][sep]: [value], so that in French we can have "Catégories : Cafés".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't say why but that's never happened before in Smoothie.
Perhaps because we rather concatenate Widgets than Strings, or because the most verbose things come directly from the server as KP.

Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@monsieurtanuki monsieurtanuki merged commit edc2b69 into openfoodfacts:develop Mar 2, 2023
@monsieurtanuki
Copy link
Contributor Author

Thank you @stephanegigandet for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants