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

Redesign request screen UI #1795

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Nagarjuna0033
Copy link
Contributor

@Nagarjuna0033 Nagarjuna0033 commented Oct 20, 2024

Issue Fix

Fixes #{Issue Number}
Jira Task: Task_Number

Screenshots

Before Theme Figma

Description

  • Apply the AndroidStyle.xml style template to your code in Android Studio.

  • Run the unit tests with ./gradlew check to make sure you didn't break anything

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

Comment on lines 36 to 42
if (titleColor != null) {
Text(
text = stringResource(id = topBarTitle),
style = MaterialTheme.typography.titleMedium,
color = titleColor,
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing logic for titleColor == null

do

  Text(
                    text = stringResource(id = topBarTitle),
                    style = MaterialTheme.typography.titleMedium,
                    color = titleColor ?: ColorHere,
                )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From mifos scaffold I am passing default color so there is chance of getting null value


@Composable
internal fun ShowQrContent(
qrDataBitmap: Bitmap,
qrDataString: String,
// qrDataString: String, // this parameter is not used in new figma design
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments

modifier = modifier
.fillMaxSize()
.background(
getColor(),
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need a separate function for this. Do it here

@@ -50,13 +51,13 @@ internal fun ShowQrScreenRoute(
viewModel: ShowQrViewModel = koinViewModel(),
) {
val uiState by viewModel.showQrUiState.collectAsStateWithLifecycle()
val vpaId by viewModel.vpaId.collectAsStateWithLifecycle()
// val vpaId by viewModel.vpaId.collectAsStateWithLifecycle()
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments

Comment on lines 109 to 110
// qrDataString = vpaId,
// amount = amount,
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well

) { Icon(MifosIcons.Share, null) }
},
modifier = modifier,
modifier = modifier.background(getScaffoldBackgroundColor()),
Copy link
Contributor

Choose a reason for hiding this comment

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

lets do it here. if (isSysteminDarktheme()) color1 else color2
also no hardcoded colors. (line206)

modifier = Modifier
.fillMaxWidth()
.height(55.dp),
onClick = { onShare() },
Copy link
Contributor

Choose a reason for hiding this comment

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

the app crashes when I click share button, can you see why?

horizontalAlignment = Alignment.Start,
.fillMaxSize(),
verticalArrangement = Arrangement.SpaceBetween,
horizontalAlignment = Alignment.CenterHorizontally,
Copy link
Contributor

Choose a reason for hiding this comment

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

enable vertical scrolling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The content of screen is not overflowing

Copy link
Contributor

Choose a reason for hiding this comment

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

@Nagarjuna0033 Try rotating your phone.

@Nagarjuna0033
Copy link
Contributor Author

@itsPronay fixed your suggestions take a look

@@ -76,7 +74,7 @@ internal fun ShowQrContent(
) {
Text(
text = stringResource(id = R.string.feature_request_money_title),
color = getColor(),
color = MifosBlue,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we still using MifosBlue? Didn’t we already change the theme?

Copy link
Contributor Author

@Nagarjuna0033 Nagarjuna0033 Oct 21, 2024

Choose a reason for hiding this comment

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

@itsPronay dark theme for this is not provided for applying same figma theme for both dark mode and light mode i have used this

Copy link
Contributor

Choose a reason for hiding this comment

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

I think since we've updated the theme for other screens, we should apply it here as well. Otherwise, it might appear inconsistent.
@niyajali what are your thoughts on this?

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