Documentation for features/location #2985
Conversation
WalkthroughAdds a multiplatform RenderMap expect/actual composable and navigation helpers for the locations feature; provides platform-specific RenderMap implementations (Android documented, others no-op) and new navigation routes/extensions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Nav as NavController/NavGraph
participant Screen as LocationsScreen
participant Map as RenderMap
User->>Nav: navigateToLocationsScreen()
Nav->>Screen: compose LocationsScreen route
activate Screen
Screen->>Map: RenderMap(modifier)
activate Map
alt Android
Map->>Map: Android actual — center Google Map on Mifos HQ (doc)
else Desktop / JS / Native / WasmJs
Map->>Map: actual implementations are no-op
end
deactivate Map
deactivate Screen
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.kt (1)
36-51: Fix layout overflow: don’t use fillMaxSize inside Column; use weight with a wrapper.Current fillMaxSize can push content off-screen. Wrap the map and allocate remaining space.
- RenderMap(modifier = Modifier.fillMaxSize()) + Box( + modifier = Modifier + .weight(1f) + .fillMaxWidth() + ) { + RenderMap(modifier = Modifier.matchParentSize()) + }Add import if missing:
import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxWidthfeature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavGraph.kt (1)
30-37: Use the multiplatform navigation artifact or move this file to androidMain.This file is in
commonMainbut importsandroidx.navigation.*, which is Android-only. Either:
- Move
LocationNavGraph.kttoandroidMain/, or- Use the MPP artifact
libs.jb-composeNavigation(org.jetbrains.androidx.navigation:navigation-compose) and declare it in thecommonMaindependencies offeature/location/build.gradle.ktsThe current setup will fail on non-Android targets.
feature/location/src/desktopMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.desktop.kt (1)
8-8: Fix the repository URL typo in the license comment.The URL contains "mobile-mobile" but should be "mifos-mobile" to match the actual repository name.
Apply this diff:
- * See https://github.com/openMF/mobile-mobile/blob/master/LICENSE.md + * See https://github.com/openMF/mifos-mobile/blob/master/LICENSE.mdfeature/location/src/nativeMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.native.kt (1)
8-8: Fix the repository URL typo in the license comment.The URL contains "mobile-mobile" but should be "mifos-mobile" to match the actual repository name.
Apply this diff:
- * See https://github.com/openMF/mobile-mobile/blob/master/LICENSE.md + * See https://github.com/openMF/mifos-mobile/blob/master/LICENSE.mdfeature/location/src/wasmJsMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.wasmJs.kt (1)
8-8: Fix the repository URL typo in the license comment.The URL contains "mobile-mobile" but should be "mifos-mobile" to match the actual repository name.
Apply this diff:
- * See https://github.com/openMF/mobile-mobile/blob/master/LICENSE.md + * See https://github.com/openMF/mifos-mobile/blob/master/LICENSE.md
🧹 Nitpick comments (4)
feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavGraph.kt (1)
21-23: Avoid stacking multiple copies of LocationsScreen.Use launchSingleTop and restoreState to dedupe and preserve state on reselect.
fun NavController.navigateToLocationsScreen() { - navigate(LocationsNavigation.LocationsScreen.route) + navigate(LocationsNavigation.LocationsScreen.route) { + launchSingleTop = true + restoreState = true + } }feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavigation.kt (1)
13-20: Reduce public API surface: keep sealed class public; make route consts internal.Prevents consumers from mixing two entry points for the same routes.
-const val LOCATIONS_NAVIGATION_ROUTE_BASE = "locations_base_route" +internal const val LOCATIONS_NAVIGATION_ROUTE_BASE = "locations_base_route" -const val LOCATIONS_SCREEN_ROUTE = "locations_screen_route" +internal const val LOCATIONS_SCREEN_ROUTE = "locations_screen_route"feature/location/src/androidMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.android.kt (1)
38-42: Localize marker strings.Use shared string resources instead of hardcoded literals.
- Marker( - state = MarkerState(position = headquarterLatLng), - title = "Mifos Initiative", - snippet = "Mifos Location", - ) + Marker( + state = MarkerState(position = headquarterLatLng), + title = stringResource(Res.string.mifos_initiative), + snippet = stringResource(Res.string.mifos_location), + )Add these imports:
import org.jetbrains.compose.resources.stringResource import mifos_mobile.feature.location.generated.resources.Resfeature/location/src/jsMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.js.kt (1)
21-23: Add a lightweight placeholder to avoid blank space on JS.Show a subtle placeholder so users understand the feature is unsupported on this platform.
@Composable actual fun RenderMap(modifier: Modifier) { // TODO: Replace with a real implementation androidx.compose.material3.Text( text = "Map is not available on this platform yet.", modifier = modifier ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
feature/location/src/androidMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.android.kt(1 hunks)feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.kt(2 hunks)feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavGraph.kt(2 hunks)feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavigation.kt(1 hunks)feature/location/src/desktopMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.desktop.kt(1 hunks)feature/location/src/jsMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.js.kt(1 hunks)feature/location/src/nativeMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.native.kt(1 hunks)feature/location/src/wasmJsMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.wasmJs.kt(1 hunks)
🔇 Additional comments (5)
feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavigation.kt (1)
22-35: Sealed destinations look good.
Clear, type-safe routes with data object singletons.feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.kt (1)
54-61: Expect signature is fine.Default parameter in expect with actuals omitting default is acceptable for MPP.
feature/location/src/desktopMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.desktop.kt (1)
15-23: LGTM! Desktop no-op implementation is appropriate.The no-op implementation for the desktop platform is correct for the expect/actual pattern. The KDoc clearly documents the current limitation.
feature/location/src/nativeMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.native.kt (1)
15-23: LGTM! Native no-op implementation is appropriate.The no-op implementation for the native platform is correct for the expect/actual pattern. The KDoc clearly documents the current limitation.
feature/location/src/wasmJsMain/kotlin/org/mifos/mobile/feature/location/LocationScreen.wasmJs.kt (1)
15-23: LGTM! WasmJs no-op implementation is appropriate.The no-op implementation for the WasmJs platform is correct for the expect/actual pattern. The KDoc clearly documents the current limitation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavigation.kt (1)
12-37: Consider enhancing documentation with usage context.While the current documentation is accurate and follows conventions, it could be more valuable by including brief usage examples or explaining when to use
LocationsBasevsLocationsScreen. For instance, mentioning thatLocationsBaseserves as the navigation graph entry point whileLocationsScreenrepresents the actual screen destination would provide clearer guidance.Example enhancement:
/** - * Base route for the locations feature navigation graph. + * Base route for the locations feature navigation graph. + * Use this as the starting destination when navigating to the locations feature. */ const val LOCATIONS_NAVIGATION_ROUTE_BASE = "locations_base_route"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavigation.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
🔇 Additional comments (2)
feature/location/src/commonMain/kotlin/org/mifos/mobile/feature/location/navigation/LocationNavigation.kt (2)
13-21: Documentation added for route constants.The KDoc comments clearly identify the purpose of each route constant. The documentation is concise and follows standard conventions.
23-37: Well-structured navigation documentation.The sealed class and its data objects are properly documented with KDoc comments. The use of a sealed class for navigation routes is a sound architectural choice, providing type safety for navigation destinations.
2 participants
Fixes - Jira-#Issue_Number
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