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

Fix background colors in App Bars #230 #236

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

PrakashIrom
Copy link
Contributor

I made some changes in the Material Theme and added containerColor parameter for the NavigationBar to fix the background color issue, but for the TopAppBar I don't see any differences in the screenshots you uploaded.
If there's any changes you want to make or anything wrong with what I did, please do tell me.

@lorenzovngl lorenzovngl linked an issue Oct 8, 2024 that may be closed by this pull request
Copy link
Owner

@lorenzovngl lorenzovngl left a comment

Choose a reason for hiding this comment

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

It was more difficult than expected to figure out why this bug has appeared.

Regarding TopAppBar, you're right. I'm sorry for not providing a meaningful screenshot. Please fix it as described here:

navigationIcon = navigationIcon,
scrollBehavior = scrollBehavior
)

        navigationIcon = navigationIcon,
-       scrollBehavior = scrollBehavior
+       scrollBehavior = scrollBehavior,
+       colors = TopAppBarDefaults.topAppBarColors(
+            scrolledContainerColor = MaterialTheme.colorScheme.surfaceColorAtElevation(TonalElevation.level2())
+       )
    )
}

@@ -6,7 +6,7 @@ val md_theme_light_primary = Color(0xFF934B00)
val md_theme_light_onPrimary = Color(0xFFFFFFFF)
val md_theme_light_primaryContainer = Color(0xFFFFDCC5)
val md_theme_light_onPrimaryContainer = Color(0xFF301400)
val md_theme_light_secondary = Color(0xFF755945)
val md_theme_light_secondary = Color(0xFF755945)//
Copy link
Owner

Choose a reason for hiding this comment

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

Has this been included in the commit by mistake?

@@ -23,7 +23,7 @@ val md_theme_light_onBackground = Color(0xFF201A17)
val md_theme_light_surface = Color(0xFFFFFBFF)
val md_theme_light_onSurface = Color(0xFF201A17)
val md_theme_light_surfaceVariant = Color(0xFFF3DFD2)
val md_theme_light_onSurfaceVariant = Color(0xFF52443B)
val md_theme_light_onSurfaceVariant = Color(0xFF52443B)//
Copy link
Owner

Choose a reason for hiding this comment

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

Has this been included in the commit by mistake?

@@ -38,7 +38,7 @@ val md_theme_dark_onPrimary = Color(0xFF4F2500)
val md_theme_dark_primaryContainer = Color(0xFF703800)
val md_theme_dark_onPrimaryContainer = Color(0xFFFFDCC5)
val md_theme_dark_secondary = Color(0xFFE4BFA7)
val md_theme_dark_onSecondary = Color(0xFF422B1A)
val md_theme_dark_onSecondary = Color(0xFF422B1A)//
Copy link
Owner

Choose a reason for hiding this comment

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

Has this been included in the commit by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh yeah sorry it was by mistake

@@ -63,7 +63,7 @@ private val DarkColors = darkColorScheme(
surfaceVariant = md_theme_dark_surfaceVariant,
onSurfaceVariant = md_theme_dark_onSurfaceVariant,
outline = md_theme_dark_outline,
inverseOnSurface = md_theme_dark_inverseOnSurface,
inverseOnSurface = md_theme_light_inverseSurface,
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't modify this file. Also, this is incorrect: why assigning a light-themed color to a dark-themed variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh my bad I was changing the color to see the matching bottom bar color.

@@ -48,7 +49,7 @@ fun MyBottomAppBar(
unselectedIcon = Icons.Outlined.Info
)
)
NavigationBar {
NavigationBar(containerColor = MaterialTheme.colorScheme.inverseOnSurface) {
Copy link
Owner

Choose a reason for hiding this comment

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

It's correct to change containerColor, but please assign the following value:

Suggested change
NavigationBar(containerColor = MaterialTheme.colorScheme.inverseOnSurface) {
NavigationBar(
containerColor = MaterialTheme.colorScheme.surfaceColorAtElevation(TonalElevation.level2())
) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I will check

Copy link
Owner

@lorenzovngl lorenzovngl left a comment

Choose a reason for hiding this comment

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

It was more difficult than expected to figure out why this bug has appeared.

Regarding TopAppBar, you're right. I'm sorry for not providing a meaningful screenshot. Please fix it as described here:

navigationIcon = navigationIcon,
scrollBehavior = scrollBehavior
)

        navigationIcon = navigationIcon,
-       scrollBehavior = scrollBehavior
+       scrollBehavior = scrollBehavior,
+       colors = TopAppBarDefaults.topAppBarColors(
+            scrolledContainerColor = MaterialTheme.colorScheme.surfaceColorAtElevation(TonalElevation.level2())
+       )
    )
}

Please add this change reported above to fix colors also in TopAppBar, and remove the changes I requested to remove on the previous review.
Thank you

@PrakashIrom
Copy link
Contributor Author

Okay I will do it.

@PrakashIrom
Copy link
Contributor Author

I have made the changes please do check

@lorenzovngl
Copy link
Owner

All good! Thank you for your efforts and welcome among the contributors of Food Expiration Dates! 🎉

@lorenzovngl lorenzovngl merged commit 45c31d1 into lorenzovngl:main Oct 10, 2024
2 checks passed
@PrakashIrom
Copy link
Contributor Author

Thank you @lorenzovngl once again, without your help I couldn't do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix background colors in App Bars
2 participants