Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Migrate :core:designsystem to KMP #2290

Open
wants to merge 9 commits into
base: kmp-impl
Choose a base branch
from

Conversation

biplab1
Copy link

@biplab1 biplab1 commented Jan 20, 2025

Fixes - Jira-#I334

Didn't create a Jira ticket, click here to create new.

Please Add Screenshots If there are any UI changes.

Before After

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the static analysis check ./gradlew check or ci-prepush.sh to make sure you didn't break anything

  • If you have multiple commits please combine them into one commit by squashing them.

@biplab1 biplab1 changed the title Directory restructure :core:designsystem to KMP Feat: Migrate :core:designsystem to KMP Jan 21, 2025
@@ -23,6 +21,7 @@ import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.drawscope.Stroke
import androidx.compose.ui.input.pointer.pointerInteropFilter
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't resolve the pointerInteropFilter and android.view.MotionEvent issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, we'll fix it later for now just move this file to androidMain source-set

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MifosDrawingCanvas.kt moved to androidMain source-set

import com.mifos.core.designsystem.theme.DarkGray
import org.jetbrains.compose.resources.stringResource
import androidclient.core.designsystem.generated.resources.Res
import androidclient.core.designsystem.generated.resources.core_designsystem_ic_error_black_24dp
Copy link
Author

@biplab1 biplab1 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not satisfied with this resolution of adding the string resources, it temporary resolves the issue, however, I don't think it is an optimized approach.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could change the package name in build.gradle.kts file and you've to run the command again to generate resources and update all usages with new package name..

./gradlew :core:designsystem:generateResourceAccessorsForCommonMain
designsystem/build.gradle.kts

compose.resources {
    publicResClass = true
    generateResClass = always
    packageOfResClass = "core.designsystem.generated.resources"
}

and after running the above command all generated resources can be accesbile using core.designsystem.generated.resources.core_designsystem_ic_error_black_24dp etc..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do as suggested.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done.

*/
package com.mifos.core.designsystem.theme

import androidclient.core.designsystem.generated.resources.Res
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I tried using android_client instead of androidclient, however, the generated resources in build folder has package named androidclient and not android_client

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in previous comment.

@@ -23,6 +21,7 @@ import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.drawscope.Stroke
import androidx.compose.ui.input.pointer.pointerInteropFilter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, we'll fix it later for now just move this file to androidMain source-set

@@ -80,7 +75,7 @@ fun MifosOutlinedTextField(
Icon(
imageVector = icon,
contentDescription = null,
tint = if (isSystemInDarkTheme()) White else DarkGray,
tint = if (isSystemInDarkTheme()) onPrimaryLight else secondaryLight,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line, no need to add any direct color for different modes, it will automatically applied based on theme mode.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line removed.

@@ -90,7 +85,7 @@ fun MifosOutlinedTextField(
maxLines = maxLines,
singleLine = singleLine,
colors = OutlinedTextFieldDefaults.colors(
focusedBorderColor = if (isSystemInDarkTheme()) BluePrimaryDark else BluePrimary,
focusedBorderColor = if (isSystemInDarkTheme()) primaryDark else primaryLight,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this too, and recheck every composable if we are applying color directly instead of material theme remove that, no need to configure such thing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the mentioned line, I will check the other composables.

Text(
text = "Invalid OTP",
style = MaterialTheme.typography.bodyMedium,
color = Color.Red,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this too

Copy link
Author

@biplab1 biplab1 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the line. Just to let you know that this was taken from kmp-template, this has also been used in mifos-mobile :core:designsystem

.wrapContentHeight(align = Alignment.CenterVertically),
text = char,
style = MaterialTheme.typography.headlineSmall,
color = if (isFocused) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Material Color like for focused use secondary and if not focused use teritory/surface color etc

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used Material Color as you suggested.

@@ -74,7 +70,7 @@ fun MifosCircularProgress(
.height(60.dp)
.padding(8.dp),
strokeWidth = 4.dp,
color = DarkGray,
color = secondaryLight,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the line.

@niyajali
Copy link
Collaborator

niyajali commented Jan 27, 2025

@biplab1 I'm commenting on one thread since you had to change all composable, and I've commented on a few components above

  • Remove explicitly/directly defined colors from all composable, if we're fully customizing or creating our own component for eg creating our own customized TextField instead of the default one, then use material colors
  • use ImageVector instead of Painter since It's marked as Stable which means it'll survive recomposition
  • Use Material Typography instead of like in some files we're defining font size as .sp instead we could use MaterialThem.typography.body/label etc based on the composable
  • If we're defining size in official composable for example, in TopBar title we're providing 24.sp, those things are unnecessary because all colors, and typography are configured already in TopBar composable no need to provide it explicitly.
  • Remove all explicitly defined colors from icon/ text/image etc or any composable that we're using official one and customize a little bit

@biplab1
Copy link
Author

biplab1 commented Jan 29, 2025

@niyajali I am working to implement all your suggestions.

@biplab1
Copy link
Author

biplab1 commented Jan 29, 2025

I couldn't find the component related to 4th point. Rest of the points I think I have implemented.

@biplab1 biplab1 marked this pull request as ready for review January 29, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants