AndroidMakers breaks sessions have "service" type causing them to be displayed in white even in dark mode about confetti

[mdupierreux]

Ok, so for breaks and services we shouldn't have the bookmark option ? From what I see, the bookmark option is visible because the breaks for Android Makers have the service type. If the type was correct, the option wouldn't be visible.

if (!session.isBreak()) {
            Bookmark(
                isBookmarked = isBookmarked,
                onBookmarkChange = { shouldAdd ->
                    if (!isLoggedIn) {
                        showDialog = true
                        return@Bookmark
                    }
                    if (shouldAdd) {
                        addBookmark(session.id)
                    } else {
                        removeBookmark(session.id)
                    }
                }
            )
        }

[mdupierreux]

You can check this pull request for the code I wrote : #1314

[joreilly]

That's merged now

[joreilly]

ah, thanks....that does look pretty ugly all right :)

[mdupierreux]

If service sessions are white (on light mode) what would be the correct color in dark mode ?

[joreilly]

I"m not sure tbh what best colour would be to use there (am definitely not a designer :) ) ....maybe one of standard ones with some change in alpha or perhaps tonalElevation as we do with ConfettiHeaderAndroid for example

[mdupierreux]

Something like this for example ?

[joreilly]

Yeah, that definitely looks better....also btw just noticed we show bookmark option for those which I guess we shouldn't!

[BoD]

Sorry, I'm also definitely not a designer 😅 - but on the proposed screenshots above, it looks like the break/service sessions are actually highlighted and have an importance higher than the other sessions whereas, arguably, it's the opposite 😅

They should probably have the same color as the background instead.

[mdupierreux]

Ok 😄, I'm going to try to find a dimmer color (background on dark theme gives me a full black list 😅 )

[joreilly]

I'm not super familiar with options here but wondering what tonalElevation might add

[mdupierreux]

I will look into it 👍

[mdupierreux]

Here's with a tonalElevation of 8dp for sessions and 0dp for services :

[joreilly]

That definitely looks better. @BoD what you think?