Moving the Building Levels quest form to Jetpack Compose.

[GaeaKat]

No description provided.

[GaeaKat]

Spotted a couple flaws, fixing

[westnordost]

Hey, thank you for that contribution!

For being new to Compose, you are already doing a lot of things correctly, IMO.

Here's a small pre-review, without having looked at how it looks like in the app:

1. Some formatting could be improved. I think ktlint could help you there, @FloEdelmann is the expert regarding that. Things like space after if, space before {, space around operators like !=, space after , etc

2. For things like buttons (AddBuildingLevelsButton), try to use standard components. Doesn't using Button as the container for the row of text, image, text produce a satisfactory result? The issues with defining your own shapes resembling buttons are amongst other things 1. doesn't look right in dark mode, 2. no tap (ripple) feedback, 3. isn't changed automatically when app-wide theme changes are made (such as using Material 3 instead of Material 2)

3. avoid specifying the font size directly, use styles wherever possible, e.g. style = MaterialTheme.typography.caption or MaterialTheme.typography.body2 (See the Typography class for the available text styles in Material2). Same goes for other styling, e.g. regarding colors (for errors etc).

4. Are you sure about wrapContentSize/wrapContentWidth? I think it is superfluous / not doing what you think it does.

5. don't put compose code into fragments. So there's this levels+roof levels input field? That's one widget, put it into an own file! So there is this form consisting of the mentioned input field plus a row of previous-selection buttons? Put that into yet another file! There's that row of previous-selection buttons? Put that at least into its own function.

6. For the levels+roof levels input, you want to hoist the state, see https://developer.android.com/develop/ui/compose/state#state-hoisting . I.e. the function should take the levels as normal String parameters and instead have a function onValuesChanged or similar to report the changed value(s) to the parent. Basically, how TextField works, too.

[GaeaKat]

re 5: so split it out into sections a bit more? got it.
6. I must admit, I was worried I would end up with like levels:Int, onLevelsChanged: (Int)->Boolean, roofLevels: Int, onRoofLevelsChanged: (Int)->Boolean which now I look at it, doesn't look as bad as I thought

[GaeaKat]

My attempt at using Button lead to this, and it was more time and code removing the default things, then adding clickable to a row, especially as according to docs and testing, using the clickable modifier does the ripple feedback. though using the theme more so stuff like dark mode works is needed.

[westnordost]

Well, you have several paddings there in nested composables (of which some are not necessary, by the way).
That the button is blue, is I think okay, that's the material style after all.

But if it really doesn't work out at all, you should use Surface, see e.g. MapButton.

[GaeaKat]

This is what I end up with after I have trimmed the button down / using Button component.

[GaeaKat]

Converting this back to draft while I work on the issues presented. thankyou for the points! :)

[GaeaKat]

Moved out of draft, ran ktlint over the files I changed now, moved it to it's own file, and moved the row to it's own file. and moved MaterialTheme.color and MaterialTheme.Typography

[westnordost]

So, is this ready to review/merge? Or do you want to review it yourself, first? Some small things like formatting etc. one might notice oneself.

[GaeaKat]

Should be ready I think

[GaeaKat]

Thankyou for the reviewed changes @FloEdelmann you are right that looks much better!

[westnordost]

Overall, it looks pretty good! You did a lot of things in Compose right, right away.

There are a many small things, so I rather worked on these myself then to complain about (and explain) all these. If you like to learn, have a look at what I changed.

Some general things specially for StreetComplete:

• max line width is 100 chars
• previews should be private
• I usually add a short documentation comment for every public composable
• formatting should be consistent: if it fits in one line, fine, if not, expand not like this

Row(modifier = Modifier
    .height(52.dp), verticalAlignment = Alignment.CenterVertically) {

but like this

Row(
    modifier = Modifier.height(52.dp),
    verticalAlignment = Alignment.CenterVertically
) {

Some general things for Compose:

• gotta check if it works well in right-to-left layouts.
  
• for paddings between items in a Row, Column, LazyRow etc., use the *Arrangement parameter (e.g. horizontalArrangement = Arrangement.spacedBy(16.dp))
• public composables should always have a Modifier parameter, for consistency. This parameter should always be passed to the immediate child composable
• The items function in LazyRow and LazyColumn has an overload that allows you to directly pass a List, so that's a little more convenient than with indices
• check out https://developer.android.com/develop/ui/compose/layouts/intrinsic-measurements for e.g. expanding the height of an element to the maximum height within a row. Very handy. The article explains why fillMaxHeight() is often not what one wants

[westnordost]

I didn't review the AddBuildingLevelsForm yet. I do think something needs to be changed where the compose world meets the Android world, but to say something definitive, I need to look closer myself, which i will do later.

But the outline is:

• values for the text fields should be TextFieldValues
• for the form, it shouldn't matter whether the input is changed by typing or by clicking on the buttons. (I.e. one callback is enough)
• the trick with errors is to not even allow in the first place for non-numbers to be entered

[westnordost]

Note to myself: It would be great if there was some indicator whether a list of "favourite" buttons like the BuildingLevelsButton can be scrolled horizontally.

How is this usually indicated? How is it usually done one iOS? If there is no common standard, look for if someone else already implemented something like this (if the lib is popular, that can become the standard). Otherwise, implement it oneself, can be used for many scrollable views, also in the tutorial etc.

Ideas for how it could look like, if no standard exists:

1. a fade-out (to-background-color, or if possible, to alpha=0)
2. a break, as seen in technical drawings , see also
   

[burrscurr]

It would be great if there was some indicator whether a list of "favourite" buttons like the BuildingLevelsButton can be scrolled horizontally.

How is this usually indicated? How is it usually done one iOS?

I had a look at some of the iOS apps on my phone and found two occurrences of a horizontally scrollable list of items. First was in the github app, second one in Overcast (which uses SwiftUI). In both cases, the list items are simply clipped at the right edge of the screen. If the gap between list items is smaller than the usual padding to the screen edge, there should always be a visual indication that there are more items available when scrolling.

Here is a screenshot of Overcast, both the recent episodes at the top and the playlists section below can be scrolled horizontally.

I think this approach is both simple and functional (I'm no UI designer though).

[mnalis]

I think this approach is both simple and functional (I'm no UI designer though).

But it won't really work unless the item are made / positioned in such a way that the last visible one never fits on the screen if there are more entries after it -- i.e. for it to be intuitive, part of the rightmost item must always be obscured / falling off the screen (on every screen size), or almost nobody will ever even try horizontal scrolling there if it looks like there is just a normal fixed row...

For example, in this EveryDoor example, in House address like (:house:), it is obvious that there is a scrollable list, but if that credit card line (:credit_card:) were just a few pixels shorter, it would've been completely undiscoverable (and yes, for some time I didn't know I could scroll for more answers).

---

I think maybe fadeout of rightmost answer (perhaps with arrow pointing to the right to make it explicit) would be more intuitive.

[westnordost]

Okay, so the result of that little excursion is that also on iOS, there is no standard for indicating that a view is scrollable. The only thing that could be done is to flash a scrollbar (or always display it). That may be an option, but not currently, because scroll bars are not supported yet by Compose.

Anyway, @GaeaKat , would you like to look into the stuff I outlined yourself, or would you rather move on and leave it to me?

[GaeaKat]

I am sorry I must admit I think I misread things! I thought your comment was about stuff you added in the rework you did. I take it you mean #6022 here?

[westnordost]

I meant #6022 (comment) just now