Added app_ar.arb to support Arabic translation ... #708
Conversation
…to support RTL layout. Not very familiar with Flutter code, so please review
adding the Arabic file
|
fix #699 |
There was a problem hiding this comment.
Thanks @naderm11 for the contribution!
The main thing we'll want adjusted in these changes before we can merge them is to take each of the different ideas that are involved here and separate them into their own commit, with their own commit message explaining what they're for. You can read all about that here, and more briefly here; but I've also left specific comments below about what that means for this PR.
| localizationsDelegates: ZulipLocalizations.localizationsDelegates, | ||
| localizationsDelegates: const [ | ||
| ...ZulipLocalizations.localizationsDelegates, | ||
| GlobalMaterialLocalizations.delegate, |
There was a problem hiding this comment.
Interesting, yeah, this looks like probably a needed change.
Just to confirm my understanding: what happens if you leave this change out?
Let's put this change in a commit of its own, perhaps at the start of the branch. (And the commit message can have the answer to my question about why it's needed.)
| ...ZulipLocalizations.localizationsDelegates, | ||
| GlobalMaterialLocalizations.delegate, | ||
| GlobalWidgetsLocalizations.delegate, | ||
| GlobalCupertinoLocalizations.delegate, |
There was a problem hiding this comment.
nit: place in logical order:
| ...ZulipLocalizations.localizationsDelegates, | |
| GlobalMaterialLocalizations.delegate, | |
| GlobalWidgetsLocalizations.delegate, | |
| GlobalCupertinoLocalizations.delegate, | |
| ...ZulipLocalizations.localizationsDelegates, | |
| GlobalMaterialLocalizations.delegate, | |
| GlobalCupertinoLocalizations.delegate, | |
| GlobalWidgetsLocalizations.delegate, |
If our app-specific delegates go first, then the next-most-specific should go next, and so on to the most general delegate going at the end. The flutter/material package depends on flutter/cupertino which depends on flutter/widgets, so that gives the ordering.
| @@ -129,7 +135,11 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver { | |||
| (_) => widget._declareReady()); | |||
| } | |||
| GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context); | |||
| return child!; | |||
| final locale = Localizations.localeOf(context); // Get locale from context | |||
There was a problem hiding this comment.
nit: this comment says only information that's equally readable from the code itself, so just leave it out:
| final locale = Localizations.localeOf(context); // Get locale from context | |
| final locale = Localizations.localeOf(context); |
| return Directionality( | ||
| textDirection: locale.languageCode == 'ar' ? TextDirection.rtl : TextDirection.ltr, |
There was a problem hiding this comment.
Huh, is this needed? I would have thought that Flutter would supply this automatically, in MaterialApp (and WidgetsApp which it's built around).
Looking in the Flutter source, it sure does look that way. It's also mentioned in the docs of Localization, which is one of the things WidgetsApp provides.
Suppose you leave this change out; how does the behavior change?
(If we do need a version of this change, it'll be another example of a change that should be its own separate commit.)
| @@ -1,4 +1,5 @@ | |||
| { | |||
| "@@locale": "en", | |||
There was a problem hiding this comment.
Hmm interesting. What's the effect of this / what led you to add it?
|
@naderm11 Do you think you'll be able to return to this PR and answer the questions above? It'd be great to have this completed and merged. |
|
This would be helpful indeed. 🙂 Updated the description with |
|
Closing this PR for now in order to clear it from our review queue. @naderm11 if at some point you'd like to pick this PR back up, please feel free to re-open it after addressing the feedback above. |
and modified app.dart to support RTL layout. Not very familiar with Flutter code, so please review :)
Fixes: #803