MM-395: Improve Dashboard Top Account List Accessibility#3113
Conversation
📝 WalkthroughWalkthroughIntroduces accessibility semantics and content-description strings to dashboard and home UIs, updates several home string labels, and adds two dashboard string resources. Also introduces unresolved Git merge conflict markers at the end of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 5
🤖 Fix all issues with AI agents
In `@cmp-shared/cmp_shared.podspec`:
- Line 53: The podspec uses Windows backslashes in the resource path which
breaks CocoaPods on macOS/Linux; update spec.resources (the array entry
currently 'build\compose\cocoapods\compose-resources') to use POSIX forward
slashes ('build/compose/cocoapods/compose-resources') so CocoaPods can resolve
compose-resources correctly.
In
`@core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/component/MifosDashboardCard.kt`:
- Line 85: The Box in MifosDashboardCard is assigned Role.Button but has no
clickable modifier; either make the card actually clickable by adding a
clickable Modifier and wiring an onClick handler (expose an onClick lambda on
MifosDashboardCard and apply Modifier.clickable within the Box), or
remove/change the semantic role to a non-actionable role (e.g.,
Role.Image/Role.Group or omit Role) so assistive tech is not misled; update the
Box semantics accordingly.
- Around line 83-89: The current use of clearAndSetSemantics inside
MifosDashboardCard (the modifier chain that includes
.focusable().clearAndSetSemantics { ... }) removes descendant semantics and
makes the eye IconButton (the visibility toggle) inaccessible; replace
clearAndSetSemantics with semantics(mergeDescendants = true) while preserving
the role and contentDescription logic so children remain actionable and the
descriptive text still applies to the combined node; update the modifier on the
dashboard card (the block that sets role = Role.Button and contentDescription
for totals) to use semantics(mergeDescendants = true) and keep focusable() so
the IconButton at lines ~165-176 remains reachable by screen readers and
keyboard.
- Around line 86-88: The contentDescription in MifosDashboardCard.kt currently
embeds hardcoded English text; replace the concatenated literal with a localized
stringResource call (e.g., stringResource(R.string.dashboard_totals, loanAmount
?: "zero", currency ?: "", savingsAmount ?: "zero", currency ?: "")) and add
corresponding string resource(s) with placeholders for loan amount, currency and
savings amount; update the contentDescription assignment in the
MifosDashboardCard composable to use stringResource(...) instead of the
hardcoded sentence.
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Line 143: Replace the hardcoded contentDescription = "Notifications" in
HomeScreen (the Icon/IconButton that sets contentDescription) with a
stringResource lookup; add an entry like "notifications" to strings.xml and use
contentDescription = stringResource(R.string.notifications) and import
androidx.compose.ui.res.stringResource so the composable uses the localized
resource instead of a hardcoded English string.
🧹 Nitpick comments (1)
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/component/MifosDashboardCard.kt (1)
84-89: Inconsistent indentation insideclearAndSetSemanticsblock.
contentDescriptionis indented to a different level thanrole. Also, the opening brace style is inconsistent (no space before{).🧹 Formatting fix
- .focusable() - .clearAndSetSemantics{ - role = Role.Button - contentDescription = - "Total loan ${loanAmount ?: "zero"} ${currency ?: ""}." + - "Total savings ${savingsAmount ?: "zero"} ${currency ?: ""}." - } + .focusable() + .clearAndSetSemantics { + role = Role.Button + contentDescription = + "Total loan ${loanAmount ?: "zero"} ${currency ?: ""}. " + + "Total savings ${savingsAmount ?: "zero"} ${currency ?: ""}." + }
|
@himanshurajputk123 Welcome to the Mifos community ! If you are looking to contribute to Mifos projects, then have a look into the onboarding guide https://mifosforge.jira.com/wiki/spaces/MP/pages/4537024513/Welcome+to+the+Mifos+Mobile+Apps+Community . You can join the slack channel and signup for daily standups. Link for the same are mentioned in the link. As per this PR, it seems that the ticket is not yet assigned to you. you can ask for it in the respective channel. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@feature/home/src/commonMain/composeResources/values/strings.xml`:
- Line 57: Update the string resource named feature_home_no_active_accounts to
use the plural form "accounts" instead of "account" so the message reads "You
don't have any active accounts yet."; locate the <string
name="feature_home_no_active_accounts"> entry and change its text accordingly to
preserve grammatical correctness across the UI.
🧹 Nitpick comments (1)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
210-223: Consider addingfillMaxWidth()to the empty-state Column.The centering currently works because
MifosAccountApplyDashboardinternally fills the width. AddingfillMaxWidth()on this Column makes the intent explicit and guards against future changes to the inner component's layout.♻️ Suggested improvement
} else { Column( + modifier = Modifier.fillMaxWidth(), horizontalAlignment = Alignment.CenterHorizontally, ) {
|
@himanshurajputk123 If there are ui changes upload before and after and resolve coderabbitai changes |
|
@Nagarjuna0033 Sure sir i'm working on it. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
core/ui/src/commonMain/composeResources/values/strings.xml (1)
134-134: Currency/amount ordering may read awkwardly for prefix-symbol currencies.The format
"Total loan %1$s %2$s"produces e.g. "Total loan 900.00 $" for prefix-symbol currencies like$or€, which is unnatural for screen readers. Consider swapping to%2$s %1$s(currency before amount), or better yet, formatting the amount with its currency symbol upstream and passing a single pre-formatted string.core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/component/MifosDashboardCard.kt (1)
147-147: Remove commented-out code.The commented-out
color = KptTheme.colorScheme.secondaryis dead code that adds noise.🧹 Suggested cleanup
-// color = KptTheme.colorScheme.secondary,feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
211-224: Empty-state implementation looks clean.The centered message with the apply-dashboard widget below provides a clear call-to-action when no accounts exist. One small note: the
fillMaxWidth()modifier is missing on the wrappingColumn, so on wider screens the column may not center as expected within the parent.💡 Consider adding fillMaxWidth for consistent centering
Column( + modifier = Modifier.fillMaxWidth(), horizontalAlignment = Alignment.CenterHorizontally, ) {
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
Can anyone tell me what to do next with this PR? |
|
Hi @niyajali , |
|
@himanshurajputk123 you marked as resolved |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmp-shared/cmp_shared.podspec`:
- Around line 54-58: Remove the unresolved Git conflict markers (`<<<<<<< HEAD`,
`=======`, `>>>>>>> 45ba382e`) from the podspec and ensure the Ruby `end` token
remains once (i.e., the file should not contain any conflict delimiters); open
the cmp_shared.podspec content around where those markers appear, choose the
correct block to keep (most likely the single trailing `end`), delete the other
duplicated/conflict lines, and run a quick `pod spec lint` or `ruby -c` to
verify the podspec parses cleanly.
himanshurajputk123
force-pushed
the
feature/MM-395-dashboard-accessibility
branch
from
9ca51d5 to
68572f9
Compare
|
@himanshurajputk123 There's no Jira ticket for this PR, consider creating one first and ask maintainers to assign it to you before working or sending a pull request, @biplab1 could you create a Jira Ticket for this |
Thank you Sir for the clarification. I’ll make sure to create or get assigned a Jira ticket before starting work on a PR next time. |
5 participants
Description
This PR improves accessibility and empty state handling for the Dashboard top account section.
Changes Made:
Issue
MM-395
Screenshots / Testing
📸 Screenshots
Before Changes->
Due to the size and complexity of the project, I encountered technical issues while attempting to switch back to the base branch and run the application to capture the “before” screenshots. Despite spending considerable time troubleshooting, I was unable to successfully generate them.
I sincerely apologize for not being able to provide the “before” screenshots and appreciate your understanding.
After Changes->
Below is the screenshot reflecting the implemented accessibility improvements:
Summary by CodeRabbit