Highlights Search Screen UI #90
Conversation
(needed for Modifier.dropShadow)
app/src/main/java/com/cornellappdev/score/components/highlights/RecentSearches.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/highlights/RecentSearches.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/highlights/RecentSearches.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/highlights/RecentSearches.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/highlights/RecentSearches.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsSearchBar.kt
Show resolved
Hide resolved
| } | ||
|
|
||
| @Composable | ||
| fun HighlightsSearchBarUI( |
There was a problem hiding this comment.
What is the difference between this and the nonUI one? I think the naming is a little bit unclear since they are both in the UI, and I am curious if we need both?
There was a problem hiding this comment.
The search bar on HighlightsScreen is a dummy search bar in the sense that it's just a clickable that navigates to access HighlightsSearchScreen where the functional search bar is. I thought it was cleaner to just have a separate UI-only version of the search bar than to have to toggle the enabled parameter of the BasicTextField (that led to some other errors)
There was a problem hiding this comment.
I agree with the point about confusing naming, I can't think of a better name rn though. I'll leave a comment in the code to explain this unless you have any suggestions?
There was a problem hiding this comment.
I think the comment is good. I don't have a good name off the top of my head but maybe something like SearchEntryPointRow, but not the biggest deal as long as there is something explaining it.
app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsSearchBar.kt
Outdated
Show resolved
Hide resolved
|
|
||
| @Composable | ||
| fun HighlightsSubScreen( | ||
| sportList: List<Sport>, |
There was a problem hiding this comment.
Same comment as before for highlights screen about extracting parameters to vm. For testing purposes, you can maybe just define test variables within the composable
There was a problem hiding this comment.
Awesome work as always! The UI is looking great and matches the figma pretty well! I just have some questions and potential considerations about the approach for handling the search logic along with some nits. I think one thing though, is that the use of focusrequester in the previews seems to be causing a lint check error, so we might want to get that fixed. Other than that, I'll leave it to Zach for anything I may have missed.
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
I think Andrew got pretty much everything but have a few comments of my own. We need to resolve the lint before this gets merged!
...c/main/java/com/cornellappdev/score/components/highlights/HighlightsScreenSearchFilterBar.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsSearchBar.kt
Show resolved
Hide resolved
|
|
||
| @Preview | ||
| @Composable | ||
| private fun HighlightsSubScreenHeader() { |
There was a problem hiding this comment.
Should probably rename to have Preview at end since it currently shares the same name as the composable
| private fun HighlightsSubScreenPreview() { | ||
| HighlightsSubScreen( | ||
| sportList = sportList, | ||
| recentSearchList = listOf( |
There was a problem hiding this comment.
Nit: can probably make use of the new testing constant that was added
| .background(color = Color.White) | ||
| .padding(top = 24.dp) | ||
| ) { | ||
| Row(modifier = Modifier.padding(horizontal = 24.dp)) { |
There was a problem hiding this comment.
Nit: Can probably apply the padding to the text itself without the row for less nesting
| .background(color = Color.White) | ||
| .padding(top = 24.dp) | ||
| ) { | ||
| Surface( |
There was a problem hiding this comment.
Nit: you could probably include the Surface part in the header composable as well
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .clickable(onClick = {/*search the query*/ }) |
There was a problem hiding this comment.
Nit: could be passed in as onClick parameter
| } | ||
|
|
||
| IconButton( | ||
| onClick = {/*delete this search from the recent searches list*/ }, |
There was a problem hiding this comment.
Nit: can be passed as an onClick parameter
| interactionSource = interactionSource, | ||
| indication = null | ||
| ) { onSearchClick() } | ||
| .clickable { onSearchClick() } |
There was a problem hiding this comment.
Is this onSearchClick necessary for focusing or can we allow the basic text field focus to take care of that for us?
| }, | ||
| modifier = Modifier | ||
| .weight(1f) | ||
| .onFocusChanged { state -> |
There was a problem hiding this comment.
Not sure if this is necessary if we have the logic in the text field?
| modifier = Modifier | ||
| .padding(horizontal = 24.dp) | ||
| .clip(shape = RoundedCornerShape(100.dp)) | ||
| .onFocusChanged { focusState -> |
There was a problem hiding this comment.
Not sure if this ideal to have here or if we can just pass in some sort of onFocus function/logic to the search bar component
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| HighlightsSearchBar( | ||
| onSearchClick = { |
There was a problem hiding this comment.
Not sure if this is needed here if we have the focus change detection in the search bar itself? Maybe you can pass in an onFocus function?
| "Cancel", | ||
| style = bodyMedium, | ||
| modifier = Modifier.clickable { | ||
| isFocused = true; |
| } | ||
| } | ||
|
|
||
| @Preview |
There was a problem hiding this comment.
Nit: might be good to have a preview or preview parameter where we can see the highlights list as well instead of just recent searches
| Column( | ||
| modifier = Modifier.padding(horizontal = 24.dp) | ||
| ) { | ||
| if (recentSearchList.isNotEmpty() && query.isEmpty()) { //start state: no search attempted yet |
There was a problem hiding this comment.
Nit: It could be potentially worth considering using something like animatedcontent here to have more control over the transition between recent searches content and the search results
|
|
||
| /* Used to display number of results in on HighlightsSearchScreen*/ | ||
| @Composable | ||
| fun HighlightsCardLazyColumnNumResultsHeader( |
There was a problem hiding this comment.
Nit: probably don't need to abbreviate num or can potentially just remove num
Overview
HighlightsSearchScreen + HighlightsSubScreen
related components
Changes Made
All components for search screens complete
Updated nav bar so HighlightsScreen is accessible when app is run
Other search screens can be viewed via previews
Test Coverage
Visually matches figma
Next Steps
Code structure/logical flow def isn't perfect rn, will clean that up when I network
Screenshots
search bar functionality
score_pr90.webm