Improve Material Design spacing in LoanAccountSummaryScreen #2563
Conversation
WalkthroughAdjusts UI spacing and component dimensions in LoanAccountSummaryScreen: reduces container horizontal padding, increases vertical paddings for text and divider, enlarges Canvas padding and Spacer heights, and changes Button padding and height. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
286-312: Fix accessibility: provide meaningful contentDescription for status indicator.While the padding increase from 4dp to 8dp improves spacing consistency, the Canvas element's
contentDescriptionis set to an empty string. This Canvas draws a color-coded status indicator (green for active, yellow for pending approval, blue for waiting for disbursal, black otherwise) that conveys important loan status information visually. Screen reader users need a textual description of this status.🔎 Proposed fix for accessibility
+val statusDescription = stringResource( + when { + loanWithAssociations.status.active == true -> { + // Add appropriate string resource for "Loan is active" + Res.string.feature_loan_status_active + } + loanWithAssociations.status.pendingApproval == true -> { + // Add appropriate string resource for "Loan pending approval" + Res.string.feature_loan_status_pending_approval + } + loanWithAssociations.status.waitingForDisbursal == true -> { + // Add appropriate string resource for "Loan waiting for disbursal" + Res.string.feature_loan_status_waiting_disbursal + } + else -> { + // Add appropriate string resource for "Loan closed" + Res.string.feature_loan_status_closed + } + } +) Canvas( modifier = Modifier .size(22.dp) .padding(top = 8.dp, end = 8.dp), - contentDescription = "", + contentDescription = statusDescription, onDraw = {Note: You'll need to add the corresponding string resources to your resource files.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
🔇 Additional comments (6)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (6)
269-269: LGTM! Aligns with Material Design guidelines.The reduction from 24dp to 16dp for horizontal padding aligns with Material Design 3's standard screen edge margins, providing consistent spacing across the app.
275-277: LGTM! Improved spacing and cleaner code.The change from 8dp to 12dp vertical padding improves visual hierarchy for the client name, and the unified
verticalpadding is more concise than separate top/bottom values.
320-324: LGTM! Improved spacing consistency.The change from 2dp to 8dp aligns with the 4dp grid system and provides better visual separation between sections.
355-355: LGTM! Better visual separation.Increasing the spacer from 8dp to 12dp provides improved visual separation before the data table section.
362-362: LGTM! Improved visual hierarchy.Increasing the spacer from 4dp to 12dp provides better separation between the data table and the action button, improving visual hierarchy.
364-396: LGTM! Aligns with Material Design touch target guidelines.The button height increase from 45dp to 48dp aligns with Material Design's recommended minimum touch target size of 48dp, improving accessibility and usability. The horizontal padding increase from 8dp to 16dp also provides better spacing for button content.
|
Thanks for the review! |
There was a problem hiding this comment.
@TapanYadav000 , thank you for your contribution, but can you confirm if the changes you have made are according to the Figma file? We are currently following the Figma design and want it to stay consistent in every screen
revanthkumarJ
left a comment
revanthkumarJ
left a comment
There was a problem hiding this comment.
use paddings from DesignToken
|
Closing in reference of #2562 |
4 participants
Summary
This PR improves spacing and layout consistency in the Loan Account Summary screen by aligning paddings, spacers, and component sizes with Material Design guidelines.
Changes
Motivation
The previous layout used inconsistent spacing values (e.g. 2dp, 4dp, 6dp), which affected visual consistency and readability. These changes make the screen more polished and consistent with modern Compose UI practices.
Testing
Screenshots
N/A (visual spacing improvement only)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.