feat(feature:account): Migrate account module to CMP #2771
Conversation
| @@ -143,6 +143,7 @@ fun MifosOutlinedButton( | |||
| * clickable and will appear disabled to accessibility services. | |||
| * @param content The button content. | |||
| */ | |||
| // internal fun MifosTextButton( | |||
| import org.mifos.mobile.feature.account.utils.AccountState | ||
| import org.mifos.mobile.feature.account.viewmodel.AccountsViewModel | ||
|
|
||
| @Composable | ||
| internal fun AccountsScreen( | ||
| isNetworkConnected: Boolean, |
There was a problem hiding this comment.
Inject NetworkMonitor directory in AccountsViewModel and observe as AccountsViewModel in here and composable and use
| import org.mifos.mobile.feature.account.viewmodel.AccountsViewModel | ||
|
|
||
| @Composable | ||
| internal fun ClientAccountsScreen( | ||
| isNetworkConnected: Boolean, |
There was a problem hiding this comment.
Inject NetworkMonitor directory in AccountsViewModel and observe as AccountsViewModel in here and composable and use
|
|
||
| import androidx.compose.ui.graphics.Color | ||
|
|
||
| val White = Color(0xFFFFFFFF) |
There was a problem hiding this comment.
Why, you're adding and using these HardCoded colors, use MaterialColor, never use hardcoded colors to a Composable,
remove all hardcoded color from all composable and use MaterialColor instead
There was a problem hiding this comment.
@niyajali I completely get your point about avoiding hardcoded colors. Previously, the account module had a colors.xml file, but since Compose Multiplatform can’t access it, I created a Color.kt with the same colors and used it in the code.
If I were to move these colors into the designsystem module and set up light/dark theme configurations, it would mean changing another module, which isn’t within the scope of this PR.
That said, my idea was to keep it as is for now and, when we work on UI colors and typography in the future, we can centralize everything properly. What do you think about this approach?
There was a problem hiding this comment.
Could you tell me where we exactly using these colours, like which type of composable like it's a basic text, Badge, card etc, if so use M3 colors like, territory, secondary, error, etc we've lots of M3 colors, use those no need to use those hardcoded color or no need to move to design system.
There was a problem hiding this comment.
And we don't need to configure any design system elements like typography, colors for different modes, or for dynamic themes, since we already configured everything on project setup, and if we pass hardcoded value to any composable we've to manually configure/provide colors for different mode, otherwise it will be automatically provided/configured by M3. And it's best practices not to pass hardcoded value to any composable, we could use M3 color and provide alpha for more configuration.
And we could tell more if you provide a screenshot of that screen, so we can see how it looks, and can tell which other Jetpack composables can be used for better usability and less configuration to look the same.
There was a problem hiding this comment.
The colors are primarily used to indicate different account states like Approved, Submitted, Closed, Withdrawn, etc. They’re also applied in the Filter dialog checkboxes to help filter these accounts. I’ve attached a screenshot below for better context.
I totally get your point about following M3 principles and avoiding hardcoded colors. If you’d like, I can go ahead and apply the best-suited colors from the M3 palette based on these states, ensuring we stick to best practices.
|
@HekmatullahAmin And I don't understand what you are trying to do, Your PR title says |
HekmatullahAmin
changed the title
@niyajali Thanks for the feedback! You’re absolutely right, the PR title doesn’t reflect the changes accurately. Initially, I followed a step-by-step migration process: first, I added the required dependencies, then moved files to common, and finally migrated to CMP as needed. Because of that, the title remained the same. I’ll update it now to better reflect the changes. |
HekmatullahAmin
force-pushed
the
MM-178
branch
from
633f78c to
5ee98d4
Compare
|
@niyajali I just wanted to give you a quick update regarding the account module. I applied the Material theme there, but I kept some colors in Color.kt to ensure we get the ideal colors for checkboxes. Also, I noticed something interesting about the color palette. It seems like whoever generated these light and dark colors in the Material Theme Builder might have manually selected the primary, secondary, and tertiary colors before generating the palette. As a result, there's a bit less contrast between them than we'd ideally want. The best approach here would be to just pick the primary color and let the Theme Builder handle the rest—it does a great job generating a more balanced and contrasting palette that way. I did my best to choose those color which have a difference in light and dark theme, but I think it would be good if we change the colors in design system. 
|
|
@HekmatullahAmin I've generated those colors using the primary color only. Besides, providing different colors for checkbox is just useless and doesn't look good, use the default color since it contained a label which indicates its value and redesigns the account screen item card using |
|
|
||
| import androidx.compose.ui.graphics.Color | ||
|
|
||
| val Black = Color(0xFF000000) |
There was a problem hiding this comment.
Remove these colors we no need to provide separate colors for a feature, if we're doing like for every feature then configuring design system and M3 is just useless
|
|
||
| return accounts.orEmpty().filter { account -> | ||
| account?.let { | ||
| it.productName?.lowercase(Locale.ROOT)?.contains(searchTerm) == true || | ||
| it.accountNo?.lowercase(Locale.ROOT)?.contains(searchTerm) == true | ||
| it.productName?.lowercase()?.contains(searchTerm) == true || |
There was a problem hiding this comment.
Move all filtering logic to separate UseCase, and use IODispatcher for filtering or sorting items, this ViewModel looks messy to me.
|
Hi @niyajali I just wanted to let you know about the recent changes I made:
Regarding your review about moving the filtering logic to a separate UseCase and using IODispatcher, I totally agree with you. I also noticed the ViewModel feels a bit messy—for instance, the refresh and setFilterList methods could be simplified, and we could have a generic searchInList function instead of separate methods for searchInSharesList, searchInLoanList, and searchInSavingsList. When I picked up the task, I actually brought up the idea of cleaning things up and aligning on a pattern during stand-up. Since the Mobile Wallet project uses MVI, I asked if we should consider consistency across projects. But the response was that the primary focus right now is just to complete the migration, and we can revisit the structure and clean-up efforts later. |
… default checkbox colors
HekmatullahAmin
force-pushed
the
MM-178
branch
from
9b7dbf8 to
bd06ddb
Compare
|
@niyajali Based on what I mentioned earlier, I didn’t make changes to the ViewModel because the focus right now is on completing the migration, as discussed in the stand-up. However, if you still feel it’s important to address those changes now, please let me know, and I’ll make the updates accordingly. |
|
@HekmatullahAmin we can't merge it, as we discussed in slack create a quick pr to setup all accounts modules and setup base module with tab setup, and then send separate PR's for each module |
Fixes - Jira-#178
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.