feat: Implemented basic login #742
Conversation
There was a problem hiding this comment.
Good morning @M123-dev, I have tons of comments for you: enjoy! ;)
| @@ -42,6 +42,7 @@ android { | |||
| targetSdkVersion 30 | |||
| versionCode flutterVersionCode.toInteger() | |||
| versionName flutterVersionName | |||
| multiDexEnabled true | |||
There was a problem hiding this comment.
It's not a good sign if we need to turn the multidex on: we're getting fatter and fatter.
There was a problem hiding this comment.
Maybe we are able to finetune some minify settings but yes looks like it
packages/smooth_ui_library/lib/widgets/smooth_text_form_field.dart
Outdated
Show resolved
Hide resolved
packages/smooth_ui_library/lib/widgets/smooth_text_form_field.dart
Outdated
Show resolved
Hide resolved
packages/smooth_ui_library/lib/widgets/smooth_text_form_field.dart
Outdated
Show resolved
Hide resolved
|
Thanks for once again a very huge review @monsieurtanuki, a simple approval would certainly be more pleasant but being made to revise my code and bringing it to its best shape keeps the codebase clean, forces me to write higher quality code and I can learn from it. And isn't that what it's all about |
There was a problem hiding this comment.
Hi @M123-dev!
Not 100% convinced, feel free to ignore my comments.
| try { | ||
| rightCredentials = await OpenFoodAPIClient.login(user); | ||
| } catch (e) { | ||
| // Returning null to show a error | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Already talked about try catch: if you catch the error in the method and just return null, you propagate nothing about the error.
I would return Future<bool> and throw an exception if relevant: in the exception I can have a message like 'server down' or 'time out' or 'you don't have internet connection`.
There was a problem hiding this comment.
I now simply throw an exception, but this is not yet visible to the end user
|
You're welcome @M123-dev: I guess it's the purpose of peer review. Especially with peers from different backgrounds and with different signature strengths. |
|
Just comparing your screenshot and the mock: you could add an |
|
@monsieurtanuki, there actually isn't one in the mocks, there is just the color of the os notification bar |
Closes: #700
Added new
DaoSecuredString,UserManagementHelperandLoginPageAdded temporary login check + route to LoginPage to
UserPreferencesPageMocks:
Screenshot: