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

904: Add i18n for translations #1103

Merged
merged 14 commits into from
Oct 23, 2023
Merged

904: Add i18n for translations #1103

merged 14 commits into from
Oct 23, 2023

Conversation

steffenkleinle
Copy link
Member

@steffenkleinle steffenkleinle commented Aug 28, 2023

Short description

This PR extracts all user visible text constants to a new arb file asset and adds general support for i18n.

Proposed changes

  • l10n/localization is setup
  • All text constants are extracted to a new arb file
  • All former text constants are now translated using l10n

Fixes #904.


@michael-markl
Copy link
Member

Just a general comment: Did you consider using the "standard flutter" approach of localization as described in their docs?
https://docs.flutter.dev/ui/accessibility-and-localization/internationalization

@steffenkleinle
Copy link
Member Author

Just a general comment: Did you consider using the "standard flutter" approach of localization as described in their docs? https://docs.flutter.dev/ui/accessibility-and-localization/internationalization

@michael-markl tbh just because I am used to react-native where you need a library for everything, so I didn't even check.

However, one thing that does not work with the standard flutter approach: It uses .arb files intstead of normal .json files which doesn't support namespaces. What would works is keys like categories_culture or similar with underscores.

Opinions on whether we should use the standard way and work around the namespace issue or stick to the current library? @michael-markl @f1sh1918 @sarahsporck

@michael-markl
Copy link
Member

@steffenkleinle According to the ARB spec (see https://github.com/google/app-resource-bundle/wiki/ApplicationResourceBundleSpecification#arb-namespace-and-registration) there might be some namespace support, however, I don't know whether the Flutter implementation supports this.

@michael-markl
Copy link
Member

One benefit of the ARB-way would be "type"-safety (in the sense that you cannot misspell translation keys) as special Dart code is generated from the ARB file (if I understand correctly).

(It's not a recommendation from me, but I am kind of interested in these ARB files. I like that (a) the translation files have some proper spec, and (b) that translation keys can also hold meta data like a description or a screenshot).

@steffenkleinle
Copy link
Member Author

steffenkleinle commented Oct 16, 2023

One benefit of the ARB-way would be "type"-safety (in the sense that you cannot misspell translation keys) as special Dart code is generated from the ARB file (if I understand correctly).

(It's not a recommendation from me, but I am kind of interested in these ARB files. I like that (a) the translation files have some proper spec, and (b) that translation keys can also hold meta data like a description or a screenshot).

Agreeing with all the points. One drawback would be that the tools we use in the integreat project to handle constants won't work anymore/would need some adjustments. I think I would still give those ARB files a try, because I think the typesafety is a huge pro.

@steffenkleinle
Copy link
Member Author

A few notes, while in general the localization works pretty nice (typesafety 🎉), it seems like no features are really implemented at the moment and there are a few problems for us:

All in all it seems like a great tool in general but not that actively maintained/improved. It might work for us for now but also might have too many limitations in the future.

@steffenkleinle steffenkleinle marked this pull request as ready for review October 18, 2023 09:07
@steffenkleinle
Copy link
Member Author

This should work so far, if you change something in app_en.arb, it should show up in the app (if your system language is set to English).

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Tested on android and ios seem to work fine for both even they recommend in the docs to add this into Info.plist.
Seem not to be require anymore.

<key>CFBundleLocalizations</key>

<array>

    <string>en</string>

    <string>ar</string>

</array>

Even tested using different system languages on device

frontend/lib/about/about_page.dart Show resolved Hide resolved
frontend/lib/about/license_page.dart Show resolved Hide resolved
frontend/lib/category_assets.dart Outdated Show resolved Hide resolved
frontend/lib/home/home_page.dart Show resolved Hide resolved
e,
);
} on QrCodeParseException catch (e) {
await _onError(
context,
'Der Inhalt des eingescannten Codes kann nicht verstanden '
Copy link
Contributor

Choose a reason for hiding this comment

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

does this break lines like before? I think each quote part is for one line
But i think its also fine if it breaks automatically. Maybe only on tablets an issue

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it doesn't lead to the same linebreaks anymore. Let me know if I should readd them. Personally I think its perhaps better to let flutter handle it depending on the screen size?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will keep it as it is :)

# Conflicts:
#	frontend/lib/identification/card_detail_view/card_detail_view.dart
#	frontend/lib/identification/identification_page.dart
Copy link
Contributor

@sarahsporck sarahsporck 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!

frontend/lib/category_assets.dart Outdated Show resolved Hide resolved
@steffenkleinle steffenkleinle merged commit 506623f into main Oct 23, 2023
1 check passed
@steffenkleinle steffenkleinle deleted the 904-constants branch October 23, 2023 13:42
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.

[Sozialpass] Setup i18n/l10n
4 participants