Regarding the message

Question

Regarding the message <https github com joreilly Confetti pull 539 issuecomment 1500044318|here> bringing the discussion here as that PR is merged now and will most likely will be hard for others to follow the comments

Stylianos Gakis · Answer

gt 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 gt 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 <https github com joreilly Confetti pull 539|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

John O&apos;Reilly · Answer

yeah agreed

John O&apos;Reilly · Answer

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

John O&apos;Reilly · Answer

but could have missed some logic

John O&apos;Reilly · Answer

I just tried NowInAndroid and they show nav rail for phone in landscape

Stylianos Gakis · Answer

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

John O&apos;Reilly · Answer

Screenshot 20230407 095717 png

John O&apos;Reilly · Answer

I ll take a look shortly and compare logic

John O&apos;Reilly · Answer

ok this is NowInAndroid ``` var shouldShowSettingsDialog by mutableStateOf false private set val shouldShowBottomBar Boolean get = windowSizeClass widthSizeClass == WindowWidthSizeClass Compact```

John O&apos;Reilly · Answer

and this is what we have ``` val shouldShowBottomBar Boolean get = windowSizeClass widthSizeClass == WindowWidthSizeClass Compact || windowSizeClass heightSizeClass == WindowHeightSizeClass Compact val shouldShowNavRail Boolean get = shouldShowBottomBar```

John O&apos;Reilly · Answer

I ll update to use NowInAndroid logic

John O&apos;Reilly · Answer

Screenshot 20230407 101402 png

John O&apos;Reilly · Answer

John O&apos;Reilly · Answer

< Stylianos Gakis> do you think this effects how we need to handle insets now

John O&apos;Reilly · Answer

see difference between us and NiA in screenshots above

mbonnin · Answer

Not to derail the discussion but do we want landscape on phones The density of information in the screenshot above is not great

mbonnin · Answer

Video is the exception but usually I like my apps to stay landscape on my phone so I can read in bed slightly smiling face

John O&apos;Reilly · Answer

yeah I tend to have landscape disabled on my phone for exactly that reason smiley

Stylianos Gakis · Answer

gt do you think this effects how we need to handle insets now gt 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 sweat smile

mbonnin · Answer

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

John O&apos;Reilly · Answer

the other option perhaps is to have more compact grid

mbonnin · Answer

Or more compact grid

mbonnin · Answer

In my experiments with apps Spotify is one of the few to actually tweak the layout a bit

mbonnin · Answer

They have player on the left and tracks albums on the right to use that horizontal real estate

mbonnin · Answer

Either case food for thought no need to do this now slightly smiling face

Stylianos Gakis · Answer

Yeah I agree This can get as complicated as we want it to especially the more we talk about it sweat smile

John O&apos;Reilly · Answer

I think it works nicely for speakers layout

John O&apos;Reilly · Answer

Search would also need to be updated for this layout

John O&apos;Reilly · Answer

Title bar probably taking up too much space here as well

Stylianos Gakis · Answer

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> sweat smile it at least feels less confined as it goes under the system nav bar too imo

John O&apos;Reilly · Answer

re app bar we have this right now ``` val titleFontSize = if appState isExpandedScreen 40 sp else MaterialTheme typography titleLarge fontSize```

John O&apos;Reilly · Answer

tried changing this here but app bar still has larger height than I think it should

Stylianos Gakis · Answer

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

Stylianos Gakis · Answer

The height 64 dp is fixed and is specified 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 ```Box Modifier windowInsetsPadding WindowInsets safeDrawing only 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 before but there were some issues with it since it s commented out

John O&apos;Reilly · Answer

Seems much the same in the NowInAndroid screenshot above