Conversation
# Conflicts: # app/src/main/java/com/washingtondcsquad/tudee/di/viewModelModule.kt
kareem-01
requested review from
Ahmedsayed0895,
HendSayed25,
Mohamed-Boumaraf,
Mohamedragabali,
karrar-abbas,
khaledeid1k,
mahmoudkhai and
omer1998
|
|
||
| import java.util.Locale | ||
|
|
||
| data class MainState( |
There was a problem hiding this comment.
name of this data class shouble be MainActivityScreenState
or MainActivityUiState
but MainState is inConsistent and not the convention for naming screen state
| import kotlinx.coroutines.launch | ||
| import java.util.Locale | ||
|
|
||
| class MainViewModel( |
There was a problem hiding this comment.
i suggest to rename this to MainActivityViewModel
| import kotlinx.coroutines.flow.map | ||
| import java.util.Locale | ||
|
|
||
| class AppPreferencesServiceImpl( |
There was a problem hiding this comment.
where isDarkModelEnabled function ?
There was a problem hiding this comment.
it exists in this file down below
|
|
||
| init { | ||
| getIsDarkTheme() | ||
| getOnBoardingState() |
There was a problem hiding this comment.
same as the above method
mahmoudkhai
left a comment
mahmoudkhai
left a comment
There was a problem hiding this comment.
good work but some little things need refactoring
| } | ||
|
|
||
| viewModel { (taskId:Int , onCancel: () -> Unit , onActionResult: (success: Boolean, message: String) -> Unit) -> | ||
| viewModel { (taskId: Int, onCancel: () -> Unit, onActionResult: (success: Boolean, message: String) -> Unit) -> |
There was a problem hiding this comment.
we changed type of task id to be TaskId alise name not int
There was a problem hiding this comment.
this concerns the EditTask feature which is out of scope for this pr so I will not be modifying it, as to not affect other's work
| package com.washingtondcsquad.tudee.domain.services | ||
|
|
||
| import kotlinx.coroutines.flow.Flow | ||
| import java.util.Locale |
There was a problem hiding this comment.
try to search for kotlin version . it's not recommended to use java libraries in domain layer.
| import kotlinx.coroutines.flow.Flow | ||
| import java.util.Locale | ||
|
|
||
| interface AppPreferencesService { |
There was a problem hiding this comment.
comment on multiple lines
| appPreferencesService.isDarkModeEnabled() | ||
| }, | ||
| onSuccess = ::onDarkThemeEnabled, | ||
| onError = {} |
There was a problem hiding this comment.
i think it always a good practice to handle error state, help in debugging and improve user experience
| import kotlinx.coroutines.flow.map | ||
| import java.util.Locale | ||
|
|
||
| class AppPreferencesServiceImpl( |
There was a problem hiding this comment.
it exists in this file down below
|
|
||
| data class MainState( | ||
| val isDarkTheme: Boolean = false, | ||
| val hasOnBoardingShown: Boolean? = null, |
There was a problem hiding this comment.
i don't see any need of nullable value for hasOnBoardingShow, because it must be initialize when the app start
|
Good job, @mahmoudkhai mentioned a good points. Solve it, and I will give you approve. |
…ng logic and initial value
# Conflicts: # app/src/main/java/com/washingtondcsquad/tudee/MainActivity.kt # app/src/main/java/com/washingtondcsquad/tudee/di/viewModelModule.kt
kareem-01
requested review from
HendSayed25,
Mohamed-Boumaraf,
mahmoudkhai and
omer1998
6 participants
add main view model and with localization support