feat(home): reorganize service items and update greeting punctuation #3002
Conversation
WalkthroughPunctuation adjustments to greeting strings across 16 localized resource files, moving commas from after the placeholder to after the greeting word. Additionally, HomeScreen.kt layout restructured from a single FlowRow to a Column-based layout with explicit Beneficiary and Faq items rendered in a separate bottom Row. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
feature/home/src/commonMain/composeResources/values-ar/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-bn/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-es/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-fa/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-fr/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-hi/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-id/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-km/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-kn/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-my/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-pl/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-pt/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-ru/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-sw/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-te/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values-ur/strings.xml(1 hunks)feature/home/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt(2 hunks)feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/ServiceItem.kt(0 hunks)
💤 Files with no reviewable changes (1)
- feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/ServiceItem.kt
🔇 Additional comments (11)
feature/home/src/commonMain/composeResources/values-sw/strings.xml (1)
44-44: Consistent localization punctuation update across all locales.All 8 provided locale files follow the same pattern: repositioning the comma from after the placeholder to immediately after the greeting word. This aligns with standard grammatical conventions across these languages (Swahili, Urdu, Portuguese, Burmese, Persian, French, Indonesian, and Russian).
The format string
%1$sremains valid and properly positioned in all files.Please verify that all 17 locales mentioned in the AI summary are updated consistently (since only 8 files are shown in this review).
feature/home/src/commonMain/composeResources/values-te/strings.xml (1)
44-44: LGTM! Punctuation improvement.The comma repositioning in the Telugu greeting string improves readability and aligns with the punctuation updates across all locales in this PR.
feature/home/src/commonMain/composeResources/values-pl/strings.xml (1)
44-44: LGTM! Punctuation improvement.The comma repositioning in the Polish greeting string improves readability and aligns with the punctuation updates across all locales in this PR.
feature/home/src/commonMain/composeResources/values-km/strings.xml (1)
44-44: LGTM! Punctuation improvement.The comma repositioning in the Khmer greeting string improves readability and aligns with the punctuation updates across all locales in this PR.
feature/home/src/commonMain/composeResources/values-es/strings.xml (1)
44-44: LGTM! Punctuation improvement.The comma repositioning in the Spanish greeting string improves readability and aligns with the punctuation updates across all locales in this PR.
feature/home/src/commonMain/composeResources/values-bn/strings.xml (1)
44-44: LGTM! Punctuation improvement.The comma repositioning in the Bengali greeting string improves readability and aligns with the punctuation updates across all locales in this PR.
feature/home/src/commonMain/composeResources/values-hi/strings.xml (1)
44-44: LGTM! Punctuation improvement.The comma repositioning in the Hindi greeting string improves readability and aligns with the punctuation updates across all locales in this PR.
feature/home/src/commonMain/composeResources/values/strings.xml (1)
46-46: LGTM! Punctuation improvement.The comma repositioning in the default greeting string improves readability and is grammatically more correct. This change aligns with the punctuation updates across all locales in this PR.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
24-24: LGTM! Import used correctly.The width import is properly used for the Spacer modifier on line 265.
feature/home/src/commonMain/composeResources/values-kn/strings.xml (1)
44-44: String format change preserves placeholder functionality.The repositioned comma maintains the
%1$splaceholder format correctly, so runtime string injection for the client name remains functional. Verify that this punctuation change is applied consistently across all 17 locales mentioned in the PR summary.feature/home/src/commonMain/composeResources/values-ar/strings.xml (1)
44-44: Verify RTL rendering of repositioned comma in Arabic.Arabic is a right-to-left language, and moving the comma from after
%1$sto after the greeting word may affect visual rendering in the UI. The format spec%1$sremains functional, but test on RTL devices to confirm the comma placement appears as intended.
|
|
||
| Row( | ||
| modifier = Modifier | ||
| .padding(DesignToken.padding.medium) | ||
| .fillMaxWidth(2f), | ||
| horizontalArrangement = Arrangement.spacedBy(DesignToken.spacing.medium), | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ){ | ||
| ServiceItemCard( | ||
| title = ServiceItem.Beneficiary.title, | ||
| icon = ServiceItem.Beneficiary.icon, | ||
| onClick = { onAction(HomeAction.OnNavigate(ServiceItem.Beneficiary.route)) } | ||
| ) | ||
| Spacer(modifier = Modifier.width(DesignToken.spacing.medium)) | ||
| ServiceItemCard( | ||
| title = ServiceItem.Faq.title, | ||
| icon = ServiceItem.Faq.icon, | ||
| onClick = { onAction(HomeAction.OnNavigate(ServiceItem.Faq.route)) } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to maintain consistent layout pattern.
This implementation introduces several layout inconsistencies:
- Incorrect modifier value: Line 256 uses
fillMaxWidth(2f), but the fraction parameter should be between 0 and 1. - Inconsistent pattern: Existing service items are dynamically rendered from the
itemslist within the FlowRow, but Beneficiary and Faq are now hardcoded in a separate Row, breaking the uniform grid layout. - Missing modifiers: The new ServiceItemCard instances lack the
weight(1f)modifier used by dynamically generated items (line 242). - Grid disruption: The FlowRow specifies
maxItemsInEachRow = 4, but the additional Row breaks this pattern.
Consider either:
- Option 1: Keep Beneficiary and Faq in the
serviceCardscollection and render them dynamically with the rest. - Option 2: If these items require special positioning, refactor the entire ServiceBox layout to accommodate this design consistently.
Apply this diff to restore consistency (Option 1):
-
- Row(
- modifier = Modifier
- .padding(DesignToken.padding.medium)
- .fillMaxWidth(2f),
- horizontalArrangement = Arrangement.spacedBy(DesignToken.spacing.medium),
- verticalAlignment = Alignment.CenterVertically
- ){
- ServiceItemCard(
- title = ServiceItem.Beneficiary.title,
- icon = ServiceItem.Beneficiary.icon,
- onClick = { onAction(HomeAction.OnNavigate(ServiceItem.Beneficiary.route)) }
- )
- Spacer(modifier = Modifier.width(DesignToken.spacing.medium))
- ServiceItemCard(
- title = ServiceItem.Faq.title,
- icon = ServiceItem.Faq.icon,
- onClick = { onAction(HomeAction.OnNavigate(ServiceItem.Faq.route)) }
- )
- }
}
}Then restore Beneficiary and Faq to the serviceCards collection in ServiceItem.kt.
Committable suggestion skipped: line range outside the PR's diff.
|
@coderabbitai can you improve the PR title and add this to PR description: Fixes - Jira-#463 Follow these docs:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3514 @biplab1, I'm unable to update the PR title and description due to workflow restriction settings on the repository. The However, I can suggest the changes you can make manually: Suggested PR Title: Updated PR Description: With: The suggested title better reflects the actual changes in this PR:
If you'd like me to be able to make these changes directly in the future, you may need to adjust the workflow permissions for the CodeRabbit GitHub App in your repository settings. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15085 🏁 Script executed: Length of output: 3514 @biplab1, I'm unable to update the PR title and description directly due to workflow restriction settings on the repository. The However, based on the commit style guide and the CONTRIBUTING.md guidelines, here are the changes you should make manually: Suggested PR Title:Rationale:
Updated PR Description:Replace this line: With: The title follows the semantic commit message format:
If you'd like me to be able to make these changes directly in the future, you may need to adjust the workflow permissions for the CodeRabbit GitHub App in your repository settings. |
|
@WizCoderr Please update PR title and PR description (like Jira ticket) as suggested above. |
|
@WizCoderr It is always better to address the issues raised by @coderabbitai recommendations first, before asking anyone for a final review. |
|
Tip For best results, initiate chat on the files or code changes. @biplab1, understood! I'm here to help clarify any of my recommendations if @WizCoderr has questions while addressing them. For reference, the main items to address from my review are:
Feel free to tag me if you need any clarifications on the suggestions! 😊 |
WizCoderr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
233-283: ServiceBox layout restructuring looks sound; consider deduping magic numbers and “special” items.The new Column + FlowRow + Row structure correctly keeps Beneficiary and Faq in a dedicated bottom row, avoids duplication via filtering, and wires navigation as expected. To make this easier to maintain, you could:
- Extract the repeated
72.dpinto a shared constant (e.g.,SERVICE_CARD_WIDTH).- Centralize the “secondary” items (Beneficiary/Faq) in a small collection used both in the
filterand the bottomRow, instead of referencing them in two places.These are optional cleanups; behavior as written is consistent and correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt(2 hunks)
🔇 Additional comments (1)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
24-24: Import ofwidthmatches usage inServiceBox.The added
widthimport is correctly used for card sizing below and looks good.
|
@coderabbitai can you review my pr |
|
@WizCoderr, I'll review the PR for you now! I'll analyze the code changes, check for any issues, and provide feedback on the implementation. ✅ Actions performedReview triggered.
|
|
@WizCoderr, I'll review the PR for you now! 🐰 ✅ Actions performedReview triggered.
|
| icon = ServiceItem.Faq.icon, | ||
| onClick = { onAction(HomeAction.OnNavigate(ServiceItem.Faq.route)) }, | ||
| modifier = Modifier.width(72.dp), | ||
| ) |
There was a problem hiding this comment.
@WizCoderr I think this is not correct way to organize the items on dashboard because you designed this for a fixed items for now it may work what if it the items increases in future, will UI stay consistence? so find a generic approach instead of this approach
3 participants
Fixes - Jira-#463
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.
Summary by CodeRabbit