desmos-labs / mooncake

The first decentralized social app based on Desmos
MIT License
46 stars 14 forks source link

Refactor/localization #110

Closed ryuash closed 4 years ago

ryuash commented 4 years ago

Description

closes #109 CHANGES:

ryuash commented 4 years ago

@RiccardoM I actually worked off your UI test branch (whoops but should be fine. I just wanted to merge in the localization first before tackling the ui tests). Anyways wanted you to take a quick look, will we ever use localization outside of UI? Ex. in one of the try/catch source implementations we hit an error and want to send a message back to the UI.

codecov[bot] commented 4 years ago

Codecov Report

Merging #110 into master will increase coverage by 1.71%. The diff coverage is 4.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   17.45%   19.17%   +1.71%     
==========================================
  Files         262      263       +1     
  Lines        6496     6613     +117     
==========================================
+ Hits         1134     1268     +134     
+ Misses       5362     5345      -17     
Impacted Files Coverage Δ
lib/ui/screens/export_mnemonic_screen/index.dart 0.00% <0.00%> (ø)
...een/widgets/home_screen_top_bar_account/index.dart 0.00% <0.00%> (ø)
...creen/widgets/home_screen_top_bar_posts/index.dart 0.00% <0.00%> (ø)
...me_screen/widgets/mnemonic_backup_popup/index.dart 0.00% <0.00%> (ø)
...reen/widgets/notifications_main_content/index.dart 0.00% <0.00%> (ø)
...main_content/widgets/notifications_list/index.dart 0.00% <0.00%> (ø)
...ications_list/widgets/notification_item/index.dart 0.00% <0.00%> (ø)
.../screens/home_screen/widgets/posts_list/index.dart 0.00% <0.00%> (ø)
...st/widgets/posts_list_loading_container/index.dart 0.00% <0.00%> (ø)
...st/widgets/posts_list_syncing_indicator/index.dart 0.00% <0.00%> (ø)
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b33cf25...8cea2f5. Read the comment docs.

RiccardoM commented 4 years ago

will we ever use localization outside of UI? Ex. in one of the try/catch source implementations we hit an error and want to send a message back to the UI.

Ideally, not. What should happen instead is we are going to throw different exceptions (represented by different classes) and then translate them into messages for the user when they are caught inside the Bloc. Also because we cannot know from the repository or source how the Bloc is going to display such errors (a popup, an error message associated to an input field, etc).

RiccardoM commented 4 years ago

@ryuash I'm not a big fan of using variable strings inside the .translate("") method. If one translation is used in multiple places, this can lead to typos all over the place if the developer is not careful enough. In order to take advantage of the compile errors, can we create a class full of static const variables that represent the different JSON keys and use them instead?

So this

 PostsLocalizations.of(context).translate("appName")

Can become this

class Messages { 
  static const APP_NAME = "appName"; 
  // ...
}

PostsLocalizations.of(context).translate(Messages.APP_NAME);

This would make sure that developers cannot do typos when specifying the message that should be used.

ryuash commented 4 years ago

@RiccardoM can merge this first if you will be touching ui anytime soon. I'm on a different branch for the actual ui testings