Advnced Filter UI #85
Conversation
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Please double check that your UI matches the Figma for future PRs before requesting my review. But thank you for your work!
| if (text.isNotEmpty()) { | ||
| Spacer(modifier = Modifier.width(8.dp)) | ||
| } | ||
| } | ||
| Text(text = text, style = bodyMedium.copy(color = White)) |
There was a problem hiding this comment.
nit: imo it makes more sense to both wrap the Text and the Spacer in text.isNotEmpty() rather than just the Spacer
| if (expanded) { | ||
| Column { |
There was a problem hiding this comment.
Have you tried having this conditional rendering within the Column, and then given the Column the animateContentSize modifier? I think it would make this look much better!
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Text(title, fontSize = 18.sp) |
There was a problem hiding this comment.
The font for this is wrong. You should be using the h2 text style for this according to the Figma. If this isn't in the code base yet, you need to add it.
All this typography should be implemented as specified by the Figma.
| var expanded by remember { mutableStateOf(false) } | ||
| var selectedOption by remember { mutableStateOf("Under $20") } |
There was a problem hiding this comment.
This non-nullable type is asserting a filter is always selected. What about when there are no filters selected? Also, I think we should be using an enum class with a display string property to store the filters, rather than just strings.
| .clickable { selectedOption = option } | ||
| .padding(vertical = 4.dp) | ||
| ) { | ||
| RadioButton( |
There was a problem hiding this comment.
You are just using the default material RadioButton. As a result the colors are wrong
| selected = (selectedOption == option), | ||
| onClick = { selectedOption = option } | ||
| ) | ||
| Text(option) |
There was a problem hiding this comment.
Text is styled incorrectly
There was a problem hiding this comment.
This file should not be committed. We should get rid of it and do the proper set up using GitHub secrets. See hustle-android for a recent example of this.
| onDismissRequest = { showBottomSheet = false }, | ||
| sheetState = sheetState | ||
| ) { | ||
| LazyColumn( |
There was a problem hiding this comment.
Nit: personally I'd prefer a regular Column since it makes the UI code easy to work with, and the list of filters will not realistically be so long as to cause performance issues.
| ButtonPrimary( | ||
| "", | ||
| painterResource(id = com.cornellappdev.score.R.drawable.advanced_filter) | ||
| ) { | ||
| onAdvFilterClick() | ||
| } |
There was a problem hiding this comment.
This button is completely off from the design. If necessary, create a new IconButton component. We need to respect the design.
An additional nit, you can pass onAdvFilterClick by reference for more concision.
One more nit, I always prefer not using shortened words like "Adv" in my variable names. To someone with no context on Score, this could be "onAdvantagedFilterClick", "onAdventurousFilterClick", etc..
It only takes a couple more characters to eliminate all of this ambiguity.
Overview
Added advanced filter button and buttonsheet.
Test Coverage
Visual Tests
Next Steps
ViewModel Logic
Related filtering logic: Related filtering logic