Use MaterialTheme colors in Loan Repayment Schedule UI #2576
Conversation
📝 WalkthroughWalkthroughUpdated UI color styling in LoanRepaymentScheduleScreen.kt to use Material Design 3 theme colors instead of hardcoded colors. Background changed from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (1)
155-163: HardcodedColor.LightGrayshould be replaced with a theme color.The PR objective is to replace hardcoded colors with MaterialTheme colors, but
Color.LightGrayremains at line 162 for the bottom bar background. This will look inconsistent in dark mode and contradicts the goal of proper theming.Consider using
MaterialTheme.colorScheme.surfaceVariantorMaterialTheme.colorScheme.surfaceContainerto match the header treatment.Suggested fix
BottomBarLoanRepaymentSchedule( totalPaid = RepaymentSchedule.getNumberOfRepaymentsComplete(periods).toString(), totalOverdue = RepaymentSchedule.getNumberOfRepaymentsOverDue(periods).toString(), tvTotalUpcoming = RepaymentSchedule.getNumberOfRepaymentsPending(periods) .toString(), modifier = Modifier .align(Alignment.BottomStart) - .background(color = Color.LightGray), + .background(color = MaterialTheme.colorScheme.surfaceVariant), )feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
391-396: Incorrect semantic color for Button text.The Button text uses
MaterialTheme.colorScheme.background, but Material3'sButtoncomposable usescolorScheme.primaryas its container color by default. For proper contrast, button text should usecolorScheme.onPrimary, notbackground.Using
backgroundhere could cause poor contrast or invisible text in certain theme configurations wherebackgroundandprimaryare similar colors.Suggested fix
Button( ... ) { Text( - color = MaterialTheme.colorScheme.background, + color = MaterialTheme.colorScheme.onPrimary, text = getButtonText(loanWithAssociations.status), ) }Alternatively, omit the
colorparameter entirely and let theButtoncomposable provide the correct default viaLocalContentColor.
🧹 Nitpick comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (1)
287-309: Text color should match the actual background semantics.The text uses
onBackground, but the actual background applied via modifier (line 162) isColor.LightGray. If the background is updated tosurfaceVariantas suggested, consider updating these text colors toonSurfaceVariantfor semantic consistency with the header.Suggested fix (if background is changed to surfaceVariant)
Text( modifier = Modifier.weight(3.4f), text = stringResource(Res.string.feature_loan_complete) + " : " + totalPaid, style = MaterialTheme.typography.bodyLarge, - color = MaterialTheme.colorScheme.onBackground, + color = MaterialTheme.colorScheme.onSurfaceVariant, textAlign = TextAlign.Start, ) Text( modifier = Modifier.weight(3.3f), text = stringResource(Res.string.feature_loan_pending) + " : " + tvTotalUpcoming, style = MaterialTheme.typography.bodyLarge, - color = MaterialTheme.colorScheme.onBackground, + color = MaterialTheme.colorScheme.onSurfaceVariant, textAlign = TextAlign.Center, ) Text( modifier = Modifier.weight(3.3f), text = stringResource(Res.string.feature_loan_overdue) + " : " + totalOverdue, style = MaterialTheme.typography.bodyLarge, - color = MaterialTheme.colorScheme.onBackground, + color = MaterialTheme.colorScheme.onSurfaceVariant, textAlign = TextAlign.End, )feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (2)
453-473: Hardcoded text colors will break dark mode support.
LoanSummaryFarApartTextItemuses hardcodedColor.Black(line 464) andColor.DarkGray(line 470) for text. These will be invisible or poorly visible on dark theme backgrounds.Consider using semantic theme colors:
MaterialTheme.colorScheme.onSurfaceoronBackgroundfor primary textMaterialTheme.colorScheme.onSurfaceVariantfor secondary/muted textSuggested fix
`@Composable` private fun LoanSummaryFarApartTextItem(title: String, value: String) { Row( modifier = Modifier .fillMaxWidth() .padding(top = 6.dp), horizontalArrangement = Arrangement.SpaceBetween, ) { Text( style = MaterialTheme.typography.bodyLarge, text = title, - color = Color.Black, + color = MaterialTheme.colorScheme.onSurface, ) Text( style = MaterialTheme.typography.bodyLarge, text = value, - color = Color.DarkGray, + color = MaterialTheme.colorScheme.onSurfaceVariant, ) } }
475-487: Hardcoded row background colors will break theming.
DataTableRowdefaults toColor.White(line 482) and usesColor.Blue.copy(alpha = .3f)for alternating rows (lines 411, 426, 441). These hardcoded colors won't adapt to dark mode and will create jarring contrast against themed surfaces.Consider using:
MaterialTheme.colorScheme.surfaceorsurfaceVariantfor row backgrounds- A theme-aware accent color for alternating rows
Suggested fix
`@Composable` private fun DataTableRow( summaryColumnTitle: String, loanColumnValue: String, amountColumnValue: String, balanceColumnValue: String, isHeader: Boolean = false, - color: Color = Color.White, + color: Color = MaterialTheme.colorScheme.surface, ) {And update the callers to use theme-aware colors:
- color = Color.Blue.copy(alpha = .3f), + color = MaterialTheme.colorScheme.primaryContainer,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt
🔇 Additional comments (4)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepaymentSchedule/LoanRepaymentScheduleScreen.kt (2)
196-218: LGTM!Using
MaterialTheme.colorScheme.onBackgroundfor the text colors is semantically correct since these rows render on the default background surface. This ensures proper contrast in both light and dark modes.
225-271: LGTM!The header correctly pairs
surfaceVariantbackground withonSurfaceVarianttext colors, following Material 3 color semantics. This ensures proper contrast across themes.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (2)
269-275: LGTM!The padding adjustments align with Material Design spacing guidelines. The 16.dp horizontal padding is a standard content margin, and the 12.dp vertical padding for the client name provides appropriate breathing room.
286-323: Spacing changes look good, but hardcoded status colors remain.The padding adjustments are appropriate. However, lines 293-308 still use hardcoded
Color.Green,Color.Yellow,Color.Blue, andColor.Blackfor the status indicator. Given this PR's objective to enable proper theming and dark-mode support, these should be considered for replacement with semantic theme colors (e.g., fromMaterialTheme.colorSchemeor custom theme extensions).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
TapanYadav000
force-pushed
the
feature/use-theme-colors-repayment-schedule
branch
from
88917e7 to
3daede1
Compare
|
Thanks for the review! I’ve replaced the remaining hardcoded color with a theme-aware surface color, rebased onto the latest development, and force-pushed the updated commits. CI has been re-triggered—happy to address any further feedback. |
|
Hey @TapanYadav000, thank you for the contribution! Can you mention the Jira ticket this PR is for. If there's none then find if the similar ticket exists or else go ahead and click here to create new. |
Thanks for the review! I searched the MIFOSAC Jira for an existing open ticket related to this change but couldn’t find a suitable one. Could you please create a Jira ticket for this PR or let me know if there’s an existing one I should link to? |
Thanks for pointing this out. This PR intentionally avoids any layout or spacing changes and only replaces hardcoded colors with theme-aware values. I agree that DesignToken-based spacing is the preferred approach and it can be handled in a separate, dedicated PR to keep concerns isolated. |
Please go through this document Welcome to the Mifos Mobile Apps Community to get started. You need to login Jira first. And can create a ticket and assign it yourself, in Mifos the permissions are not restricted. Please don't forget to join the slack community as well and give the update on the same on the appropriate channel. Also follow the PR template before raising a PR and have a look into the contributing guides. (like PR title semantics) |
Thanks for the guidance! I’ve now logged into Jira and created the ticket for this PR: |
TapanYadav000
force-pushed
the
feature/use-theme-colors-repayment-schedule
branch
from
3daede1 to
4e0bf2f
Compare
|
Rebasing is complete and unrelated spacing commits have been dropped. |
2 participants
Jira: https://mifosforge.jira.com/browse/MIFOSAC-625
Summary
This PR replaces hardcoded colors in the Loan Repayment Schedule UI with values from
MaterialTheme.colorSchemeto ensure proper theming and dark-mode support.Changes
Motivation
Using theme-aware colors improves accessibility and ensures consistent appearance across light and dark modes, aligning with Material Design best practices.
Testing
Screenshots
N/A (color/theming change only)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.