https://kotlinlang.org logo
Join Slack
Powered by
Regarding the message <here>, bringing the discuss...
# confetti
s
Regarding the message here, bringing the discussion here as that PR is merged now and will most likely will be hard for others to follow the comments
I think there might be flaw in existing logic here.....if we're showing grid layout like this we should at least also be showing nav rail instead of bottom bar....
Yeah I was wondering the same about this tbh. I think the guidelines are that for a phone in landscape mode, usually you don't show a nav-rail at all in the first place. So provided that this is true, wouldn't this kinda automatically mean that maybe the phones shouldn't be seeing this style of view in landscape mode? I do really love the way it looks, but looking at the screenshots here, and they even come from a pixel 7 Pro which has a rather big screen, it's already not that easy to actually use that grid as opposed to the normal list.
j
yeah, agreed
I need to see what logic is that we have for this....I think I more or less copied it directly from NowInAndroid at the time...
but could have missed some logic
I just tried NowInAndroid and they show nav rail for phone in landscape
s
You are correct, I was wrong about the guidance for this then. The same can be done for confetti then so that we give a bit more space to this screen. And other screens of course.
j
Screenshot_20230407-095717.png
I'll take a look shortly and compare logic
ok this is NowInAndroid
Copy code
var shouldShowSettingsDialog by mutableStateOf(false)
        private set

    val shouldShowBottomBar: Boolean
        get() = windowSizeClass.widthSizeClass == WindowWidthSizeClass.Compact
and this is what we have
Copy code
val shouldShowBottomBar: Boolean
        get() = windowSizeClass.widthSizeClass == WindowWidthSizeClass.Compact ||
            windowSizeClass.heightSizeClass == WindowHeightSizeClass.Compact

    val shouldShowNavRail: Boolean
        get() = !shouldShowBottomBar
I'll update to use NowInAndroid logic
Screenshot_20230407_101402.png
@Stylianos Gakis do you think this effects how we need to handle insets now
see difference between us and NiA in screenshots above
m
Not to derail the discussion but do we want landscape on phones? The density of information in the screenshot above is not great
Video is the exception but usually I like my apps to stay landscape on my phone so I can read in bed 🙂
j
yeah, I tend to have landscape disabled on my phone for exactly that reason 😃
s
do you think this effects how we need to handle insets now
No I think we're good. The reason I specifically had passed the consume insets modifier only when the bottom bar is shown is for exactly this case where the bar isn't shown. It should all work correctly even after this change. I don't think actually not allowing the phone user to use landscape mode is the right call regardless of what's happening. Adapting the UI to it sure, but not allowing it is a very bad experience from an app imo. One last thing I can try doing is have this grid go under the nav system bar actually, instead of having the padding at the bottom since it doesn't actually need to have that extra space, as long as it has the internal padding when scrolled all the way down. The other big decision that could be made is to simply not use the grid view in phone form factors. But those are the options pretty much 😅
m
Fair enough yea I think the grid is a bit ambitious for a phone form factor. Checked a few other apps and they stay in list view
j
the other option perhaps is to have more "compact" grid
m
Or more compact grid
In my experiments with apps, Spotify is one of the few to actually tweak the layout a bit
They have player on the left and tracks/albums on the right to use that horizontal real estate
Either case, food for thought, no need to do this now 🙂
s
Yeah I agree. This can get as complicated as we want it to, especially the more we talk about it 😅
j
I think it works nicely for speakers layout
Search would also need to be updated for this layout
Title bar probably taking up too much space here as well
s
The bar itself may become less tall, yes, while still accommodating for the profile icon of course. I won a tiny bit more screen real estate with this too https://github.com/joreilly/Confetti/pull/543 😅 it at least feels less confined as it goes under the system nav bar too imo
j
re. app bar, we have this right now
Copy code
val titleFontSize =
        if (appState.isExpandedScreen) 40.sp else MaterialTheme.typography.titleLarge.fontSize
tried changing this here but app bar still has larger height than I think it should....
s
Yeap it seems like the TopAppBar by itself takes this amount of height, even if the text is not there at all. [pic 1 + pic2]
The height 64.dp is fixed and is specified here, and on top of that, you also got the top inset paddings. Together, in phones in landscape mode it makes it look big. Adding this (code snippet below) next to it shows in [pic3] the space it takes on exactly
Copy code
Box(
    Modifier
        .windowInsetsPadding(WindowInsets.safeDrawing.only(<http://WindowInsetsSides.Top|WindowInsetsSides.Top>))
        .size(64.dp)
        .border(2.dp, Color.Red)
)
There isn’t much to be done in that case for landscape mode for it then. Options would be: • Changing the scroll behavior and have it disappear while scrolling. • Do the same but for the Pager indicators • Consider collapsing the room titles instead • Any combination of the three above • Do nothing and accept the current state Because right now ~45% of the screen real estate is taken by TopAppBar PagerIndicators RoomTitles I guess this was worked around this way before, but there were some issues with it since it’s commented out.
j
Seems much the same in the NowInAndroid screenshot above
2 Views
Open in Slack
PreviousNext
Powered by