Client Loan accounts UI #2477
Client Loan accounts UI #2477
Conversation
| @Composable | ||
| fun MifosEmptyCard( | ||
| msg: String, | ||
| msg: String = "Click Here To View Filled State.", |
There was a problem hiding this comment.
move the string to strings.xml and use stringResource
| navigateToViewAccount: (Int) -> Unit, | ||
| ) { | ||
| composable<ClientLoanAccountsRoute> { | ||
| ClientLoanAccountsScreenRoute( |
There was a problem hiding this comment.
Can be changed as follows as it is a composable func that refers to a screen not a route
| ClientLoanAccountsScreenRoute( | |
| ClientLoanAccountsScreen( |
There was a problem hiding this comment.
It was added to distinguish between the two ClientLoanAccountsScreen
| is ClientLoanAccountsEvent.MakeRepayment -> TODO() | ||
| ClientLoanAccountsEvent.NavigateBack -> TODO() | ||
| is ClientLoanAccountsEvent.ViewAccount -> TODO() |
There was a problem hiding this comment.
Using TODO() can crash the app, u can use empty {}, if the functionality for the event is not in place and leave a comment saying TODO instead.
| Column( | ||
| modifier = Modifier.padding(paddingValues) | ||
| .fillMaxSize() | ||
| .padding(start = 16.dp, end = 16.dp, top = 16.dp), |
There was a problem hiding this comment.
Use DesignToken.padding instead of hardcording padding values
| ) | ||
| } | ||
|
|
||
| Spacer(modifier = Modifier.height(16.dp)) |
There was a problem hiding this comment.
Here too, try to use the DesignToken instead of hardcoded values
| viewAssociatedLoanAccounts: (Int) -> Unit, | ||
| viewAssociatedSavingsAccounts: (Int) -> Unit, | ||
| identifiers: (Int) -> Unit, |
There was a problem hiding this comment.
Please update your pr, with latest changes from my pr.
| fun NavGraphBuilder.clientProfileGeneralDestination( | ||
| onNavigateBack: () -> Unit = {}, | ||
| upcomingCharges: (Int) -> Unit = {}, | ||
| loanAccounts: (Int) -> Unit = {}, | ||
| savingAccounts: (Int) -> Unit = {}, | ||
| loanAccounts: (Int) -> Unit, | ||
| savingAccounts: (Int) -> Unit, |
There was a problem hiding this comment.
Not an issue as it is gonna change anyways, but if you are making those parameters mandatory then place then above optional parameters.
There was a problem hiding this comment.
it is not the best practice + it is confusing when used in Navigation function, i almost thought we can't navigate to loanAccounts and savings from here.
There was a problem hiding this comment.
okay. and i deleted the below comment, I didn't way they were lamba functions.
| val dialogState: DialogState? = null, | ||
| val details: Map<StringResource, String> = emptyMap(), | ||
| val networkConnection: Boolean = false, | ||
| val showAccountChooserDialog: Boolean = false, |
There was a problem hiding this comment.
where are you using this value?
5 participants
Fixes - https://mifosforge.jira.com/browse/MIFOSAC-570
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.