feat(feature:about): Migrate about module to CMP #2761
Conversation
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
@HekmatullahAmin And there are a lot of compose library available to show open source library usages like google OSS use those instead, we no need to use WebView anymore
feature/about/src/commonMain/kotlin/org/mifos/mobile/feature/about/ui/PrivacyPolicyScreen.kt
Outdated
Show resolved
Hide resolved
feature/about/src/commonMain/kotlin/org/mifos/mobile/feature/about/ui/PrivacyPolicyScreen.kt
Outdated
Show resolved
Hide resolved
|
@HekmatullahAmin we can use this https://github.com/mikepenz/AboutLibraries |
HekmatullahAmin
force-pushed
the
MM-126
branch
from
d9c013a to
e420472
Compare
|
Hi @niyajali , I’ve implemented the changes as you suggested. Regarding the openUrl() function, it’s actually an expect function that provides platform-specific implementations to navigate to the given URL. About the library you mentioned, I brought it up during the stand-up, and Mr. @therajanmaurya advised against adding any external libraries for now. As for the openUrl implementation, I’ve commented it out because it uses Intent, which requires a Context. For a small app I worked on previously, I passed an Application instance, and it worked. However, for mifos-mobile, I can’t pass the AndroidApp instance to the openUrl function. Another potential approach could be adding a custom Application for the about module and updating its manifest, but I don’t think that would be the best solution either. Let me know your thoughts! |
|
I want to keep the ticket scope as less as possible and we will implement the intent library in different ticket |
HekmatullahAmin
force-pushed
the
MM-126
branch
3 times, most recently
from
7739aa6 to
6aa7d84
Compare
|
@niyajali If you're happy with this PR, please approve it so it can be merged. |
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
Hi, here's the few things we have to maintain all over the project, make sure you've followed the same..
@HekmatullahAmin @Nagarjuna0033 @revanthkumarJ
-
LazyColumn State:
- Always pass the
LazyColumnstate as a function parameter wherever it is used.
- Always pass the
-
Keys for LazyColumn Items:
- Ensure that every item in a
LazyColumnhas a uniquekeyprovided.
- Ensure that every item in a
-
Consistent Padding/Spacing:
- Use
16.dpfor padding and spacing consistently across the project where applicable.
- Use
-
Named Arguments:
- Always use named arguments when calling functions/class to improve readability and maintainability.
-
State Hoisting:
- Maintain and hoist state from the top-level composable to ensure better state management.
And use the above mentioned library for licence, I’ll discuss with the @therajanmaurya and explain the situation. No need to worry about this; I’ll handle it. Once the changes are implemented, let me know, and we’ll proceed with merging this PR.
|
|
||
| import android.app.Application | ||
|
|
||
| class AndroidApp : Application() { |
There was a problem hiding this comment.
Why we're creating another application for one feature, we can't do that..
|
Hi @niyajali , Thanks for reviewing my PR! I’ve made sure to apply the necessary changes based on the project guidelines you shared. Specifically: I really appreciate you sharing these best practices. Regarding the License, this is not in the scope of my PR. As seen in the aboutUsNavGraph, the navigateToOssLicense function is simply assigned and will be handled elsewhere. So, this part doesn't require changes from my side. But I will work on another PR and will implement the library you suggested me. As for why I created another application, that was just for testing purposes. In my PR, I had commented them out, and now, I have deleted them entirely. 
|
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
@HekmatullahAmin This could be merged after this PR #2772 gets merged. please wait until that is done and rebase your PR and let me know, we'll merge this one as well
| activityVersion = "1.9.3" | ||
| androidDesugarJdkLibs = "2.1.4" | ||
| androidGradlePlugin = "8.7.3" | ||
| androidGradlePlugin = "8.8.0" |
There was a problem hiding this comment.
To avoid frustrating delays and build issues during a project migration, it's best to avoid changing the Android Gradle Plugin (AGP) or Gradle version. Many developers have already cached dependencies locally, and rebuilding with different versions can significantly increase build times. For a smoother experience, consistently use the same AGP and Gradle versions. and always remember to update toolsVersion as AGP
- Implemented `openUrl` function for every platform. - Used `Intent` for Android to handle URL opening. - Added platform-specific implementations for Native, Desktop, and wasmJs.
HekmatullahAmin
force-pushed
the
MM-126
branch
from
cf5c8ff to
0515ef1
Compare
@niyajali I rabased this and resolved the conflicts. Just wanted to let you know |
3 participants
Fixes - Jira-#126
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
AboutScreen.mp4
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.