Pagination #88
Pagination #88
Conversation
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Thank you for fixing this up! The filtering implementation is greatly improved since your last attempt. The pagination code looks pretty reasonable as well. There's some clean up to do but I appreciate pushing this out even though you've been busy this week! Also, make sure to send the google-services.json file to the #score-android Slack channel.
| import com.cornellappdev.score.R | ||
|
|
||
| @Composable | ||
| fun CustomRadioButton( |
There was a problem hiding this comment.
If this is a Score branded radio button, it should be named ScoreRadioButton. Also add a preview for this.
| } | ||
|
|
||
| @Composable | ||
| fun <T : DisplayableFilter> ExpandableSection( |
There was a problem hiding this comment.
Add previews please 🙏 feel free to even use AI to generate them if you want lol
| alias(libs.plugins.androidApplication) | ||
| alias(libs.plugins.jetbrainsKotlinAndroid) version "1.9.10" | ||
| alias(libs.plugins.apollo) | ||
| id("com.apollographql.apollo") version "4.0.0" |
There was a problem hiding this comment.
Nit: I wonder if we can use a variable for this Apollo version? Since it seems to be repeated twice. Or maybe a "version reference" is the right term.
| Icon( | ||
| painter = painterResource( | ||
| id = if (selected) R.drawable.ic_radio_selected else R.drawable.ic_radio_unselected | ||
| ), |
There was a problem hiding this comment.
Just switching the color of an icon gets rid of the animation when using a radio button. We should use the Material3 Radio Button instead and customize its colors to fit the Score design system.
| contentDescription = if (selected) "Selected" else "Unselected", | ||
| modifier = modifier | ||
| .size(20.dp) | ||
| .padding(end = 8.dp) |
There was a problem hiding this comment.
Reusable components should never have margin built-in, the caller should be the one to apply any margin that they need depending on the situation.
| } | ||
| } | ||
|
|
||
| fun fetchGames() = appScope.launch { |
There was a problem hiding this comment.
It seems like the backend API spec as it is now would allow us to implement pagination in the proper way (only querying the API when the user reaches the bottom of the scroll). Is the reason we're not doing it this way now due to time constraints, or are there other challenges?
Either way, we should document why the code is written this way, and probably make a GitHub issue to use proper pagination techniques based on user scroll position in the future
| var offset = 0 | ||
| var retries = 0 | ||
|
|
||
| try { |
There was a problem hiding this comment.
For this outer try catch, what is the dangerous code that causes us to need it? I believe try catch is an anti pattern in Kotlin and should be avoided whenever possible.
| var selectedPrice by remember { mutableStateOf<PriceFilter?>(null) } | ||
| var selectedLocation by remember { mutableStateOf<LocationFilter?>(null) } | ||
| var selectedDate by remember { mutableStateOf<DateFilter?>(null) } |
There was a problem hiding this comment.
Keep in mind this state will need to be hoisted out to the VM eventually, but this is fine for now.
| onDismissRequest = { showBottomSheet = false }, | ||
| sheetState = sheetState | ||
| ) { | ||
| LazyColumn( |
There was a problem hiding this comment.
Nit: I think we could just use a Column here. Also extracting your code into a FilterBottomSheetContent Composable could help make the UI code more readable.
| IconButton( | ||
| icon = painterResource(id = R.drawable.advanced_filter), | ||
| contentDescription = "Advanced filter", | ||
| onClick = onAdvancedFilterClick | ||
| ) |
There was a problem hiding this comment.
The filter button still looks a little large / padding seems off compared to the design (sorry design is hard to see I couldn't find this button without that overlay from the bottom sheet over top of it).
| Yours | Design |
|---|---|
 |
 |
Overview
Changes Made
Test Coverage
Next Steps (delete if not applicable)
Related PRs or Issues (delete if not applicable)
Screenshots (delete if not applicable)
Screen Shot Name