davx5-ose icon indicating copy to clipboard operation
davx5-ose copied to clipboard

dark theme / black text basically unreadable on dark background

Open ArnyminerZ opened this issue 1 year ago • 2 comments

Please delete this paragraph and other repeating text (like the examples) after reading and before submitting the PR.

The PR should be in Draft state during development. As soon as it's finished, it should be marked as Ready for review and a reviewer should be chosen.

See also: Writing A Great Pull Request Description

Purpose

Adjust theme color for intro.

Short description

  • Bottom bar is the same color in dark and light theme.
  • Manually passing LocalContentColor in theme.

I've forced that the bottom bar in the intro page is always the same color. I think it looks better, and we won't have the same issue again. Also I've forced LocalContentColor in the theme, because it looks like the material theme is not passing it correctly.

Checklist

  • [x] The PR has a proper title, description and label.
  • [x] I have self-reviewed the PR.
  • [x] I have added documentation to complex functions and functions that can be used by other modules.
  • [x] I have added reasonable tests or consciously decided to not add tests.

ArnyminerZ avatar Aug 18 '24 11:08 ArnyminerZ

@ArnyminerZ Can you explain more exactly how, why and where the material theme is not passing it correctly? Is there some more information on it?

I'm not sure why. The thing is that when using the Card component above, the content color is correctly set to the one that should be in the card, but the text below is not wrapped inside any material component, only foundation. And even though we are wrapping everything inside MaterialTheme, it doesn't provide any LocalContentColor:

MaterialTheme source
@Composable
fun MaterialTheme(
    colorScheme: ColorScheme = MaterialTheme.colorScheme,
    shapes: Shapes = MaterialTheme.shapes,
    typography: Typography = MaterialTheme.typography,
    content: @Composable () -> Unit
) {
    val rippleIndication = androidx.compose.material.ripple.rememberRipple()
    val selectionColors = rememberTextSelectionColors(colorScheme)
    CompositionLocalProvider(
        LocalColorScheme provides colorScheme,
        LocalIndication provides rippleIndication,
        androidx.compose.material.ripple.LocalRippleTheme provides MaterialRippleTheme,
        LocalShapes provides shapes,
        LocalTextSelectionColors provides selectionColors,
        LocalTypography provides typography,
    ) {
        ProvideTextStyle(value = typography.bodyLarge, content = content)
    }
}

LocalContentColor is set by default to black as said here:

Defaults to Color.Black if no color has been explicitly set.

Then, the Text component, if no color is given, uses the color set by LocalContentColor, not LocalTextStyle or LocalColorScheme as said here:

Additionally, for color, if color is not set, and style does not have a color, then LocalContentColor will be used.

That's why I think the color is not passed correctly, and why I simply set the onBackground color as default in the theme and not locally for the Text element, since it's actually the correct color it should have IMO.

ArnyminerZ avatar Aug 20 '24 12:08 ArnyminerZ

Thanks for the explanation. I can't say whether we should use it then or not. Intuitively I would rather update the actual text composables with the right color, because that's where I would search for it, if I wanted to change it - given the theme displays it wrongly.

Let's see what Ricki says :)

sunkup avatar Aug 20 '24 12:08 sunkup

Should be ready, looks good

ArnyminerZ avatar Sep 06 '24 07:09 ArnyminerZ