Skip to content

Conversation

@itsPronay
Copy link
Collaborator

@itsPronay itsPronay commented Aug 31, 2025

Fixes - https://mifosforge.jira.com/browse/MIFOSAC-585

Screenrecorder-2025-08-31-21-24-53-217.mp4

@itsPronay
Copy link
Collaborator Author

@TheKalpeshPawar i think, you having issues with your internet. Can you review the code instead

Comment on lines 92 to 95
modifier = Modifier.fillMaxSize()
.padding(paddingValues)
.padding(DesignToken.padding.large),
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't padding(paddingValues) be applied before fillMaxSize() here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you meant by this, but as far as i know, filllMaxSize is usually applied before padding and all.

Copy link
Contributor

@TheKalpeshPawar TheKalpeshPawar Aug 31, 2025

Choose a reason for hiding this comment

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

shouldn't the padding values of the scaffold be applied before using full screen?

Copy link
Contributor

@TheKalpeshPawar TheKalpeshPawar Aug 31, 2025

Choose a reason for hiding this comment

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

Not an issue here, but they are for adjusting top and bottom bar heights, which needs to be applied to the content, to adjust it accordingly first.

The padding values provided to your content are only set if you set the bottomBar parameter. This unfortunately is not documented. If you set the bottomBar, the Scaffold will set the bottom padding of PaddingValues to be the height of the content that the bottomBar content uses. The other padding values (start, top, end) are currently set to zero.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alr, gonna update it

Comment on lines +156 to +167
Surface(
modifier = Modifier.fillMaxWidth(),
shape = RoundedCornerShape(
bottomStart = DesignToken.padding.medium,
bottomEnd = DesignToken.padding.medium,
),
) {
menuList.map { menuItem ->
Row(
modifier = Modifier.fillMaxWidth()
.height(DesignToken.sizes.avatarMedium)
.clickable {
onActionClicked(menuItem)
},
verticalAlignment = Alignment.CenterVertically,
horizontalArrangement = Arrangement.Start,
) {
Icon(
modifier = Modifier.padding(horizontal = DesignToken.padding.large),
imageVector = menuItem.icon,
contentDescription = "",
)
Column(
modifier = Modifier.padding(
vertical = DesignToken.padding.small,
),
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you apply the surfaceContainer color to this composable, like shown in the figma design.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are few issues with these kind of cards. I am thinking of doing this in a separate ticket because we need to fix this for all the components, and those components have been used in other screens as well.

since this PR is about client identities, touching other screens wont be a good idea... we will fix this in a single PR for all the screens, as changes are similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

ooh okay.

Comment on lines 187 to 205
IconButton(
onClick = { onAction.invoke(ClientIdentitiesListAction.ToggleSearch) },
) {
Icon(
painter = painterResource(Res.drawable.search),
contentDescription = null,
)
}

Spacer(modifier = Modifier.height(DesignToken.padding.largeIncreased))

IconButton(
onClick = { onAction.invoke(ClientIdentitiesListAction.AddNewClientIdentity) },
) {
Icon(
painter = painterResource(Res.drawable.add_icon),
contentDescription = null,
)
}
Copy link
Contributor

@TheKalpeshPawar TheKalpeshPawar Aug 31, 2025

Choose a reason for hiding this comment

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

can we do this something like this

Row(
    horizontalArrangement = Arrangement.spacedBy(20.dp),
) {
    Icon(
        painter = painterResource(Res.drawable.search),
        contentDescription = null,
        modifier = Modifier.clickable{
                 onAction.invoke(ClientIdentitiesListAction.ToggleSearch)
        }
    )
    Icon(
        painter = painterResource(Res.drawable.filter),
        contentDescription = null,
        modifier = Modifier.clickable{
            onAction.invoke(ClientIdentitiesListAction.AddNewClientIdentity)
        }
    )
}

this will remove the extra space from the right side of the add icon and make the header look like it is in the figma design.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea, ill update it

@therajanmaurya therajanmaurya merged commit 6b05e20 into openMF:development Sep 1, 2025
2 checks passed
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.

4 participants