search

ilteoood / flutter_i18n

I18n made easy, for Flutter!
MIT License
217 stars 57 forks source link

Added I18nBuilder #155

Closed Amir-P closed 3 years ago

Amir-P commented 3 years ago

Added a builder widget which will be useful in cases where we want to rebuild some part of widget tree in response to locale changes.

Example1 (This way there is no need for rootAppBuilder and setState when developer uses refresh method since MaterialApp will take care of direction changes and also the whole widget tree will rebuild):

I18nBuilder(
  translationObject: flutterI18nDelegate.translationObject,
  builder: (context, locale) => MaterialApp(
    locale: locale,
    home: MyHomePage(),
    localizationsDelegates: [
      flutterI18nDelegate,
      GlobalMaterialLocalizations.delegate,
      GlobalWidgetsLocalizations.delegate
    ],
)

Example2:

I18nBuilder(
  builder: (context, _) => Text(FlutterI18n.translate(
      context, 'label.main',
      translationParams: {"user": "Flutter lover"})),
)
ilteoood commented 3 years ago

@Amir-P IMO are also missing tests and the documentation on the README. Can you please provide them?

Amir-P commented 3 years ago

@ilteoood I'm not familiar with widget testing so it will be better if you could write that. But I will update the documentation.

Amir-P commented 3 years ago

@ilteoood Can you please check it again?

moseskarunia commented 3 years ago

Stumbled upon this PR, and decided to chime in.

It's safer for the community who already used this package if every PRs also include tests to make it less likely to cause some sort of trouble in the future. Also, tests are also useful as another form of documentation.

Therefore I encourage @Amir-P to write the test first, since I don't think @ilteoood will accept an untested PR anyway.

Hope it helps.

Amir-P commented 3 years ago

@moseskarunia Thanks for your advice. I'm aware that nothing without test should be publicly released and I suggested that since I don't have experience writing widget tests, somebody else takes care of that if is possible. From #150 you can see that this plugin already doesn't have well written tests.

ilteoood commented 3 years ago

Sorry but I don't like this kind of implementation. The instance of FlutterI18n must not be available outside, in order to avoid strange behaviour not controlled by the facade.