Fix search list #68
Closed
Fix search list #68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Renamed to `Search` because the list is included ⇒ not a "bar".
This did nothing.
:whartonearth:
The container looks better now.
This a hack to some extent, we should probably change this after we start using Scaffolds.
imashnake0
added
enhancement
This was
linked to
issues
Nov 15, 2022
Closed
The list is drawn behind the search bar now. The "best" result is at the top by default so I set padding to make the list start from just below the status bar.
FQ R class was used.
boswelja
reviewed
Nov 16, 2022
| start = dimensionResource(R.dimen.large_padding), | ||
| end = dimensionResource(R.dimen.large_padding) | ||
| ), | ||
| navController = navController |
There was a problem hiding this comment.
Can we lift navController out of SearchBar while we're here? Passing a lambda can achieve the same thing while making it slightly easier to change nav framework in the future
app/src/main/java/com/imashnake/animite/features/searchbar/Search.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/imashnake/animite/features/searchbar/Search.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/imashnake/animite/features/searchbar/Search.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/imashnake/animite/features/searchbar/Search.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/imashnake/animite/features/searchbar/SearchViewModel.kt
Outdated
Show resolved
Hide resolved
The ripple has a pill shape now.
Iannnr
reviewed
Nov 16, 2022
app/src/main/java/com/imashnake/animite/features/searchbar/SearchViewModel.kt
Show resolved
Hide resolved
Iannnr
reviewed
Nov 16, 2022
app/src/main/java/com/imashnake/animite/features/searchbar/SearchFrontDrop.kt
Show resolved
Hide resolved
Iannnr
reviewed
Nov 16, 2022
app/src/main/java/com/imashnake/animite/features/searchbar/SearchFrontDrop.kt
Outdated
Show resolved
Hide resolved
boswelja
reviewed
Nov 17, 2022
| @@ -11,7 +11,9 @@ class MediaRepository @Inject constructor( | |||
| ) { | |||
| suspend fun fetchMedia(id: Int?, mediaType: MediaType): MediaQuery.Media? { | |||
| // TODO: Is there a better way to "wait for animations to complete"? | |||
| delay(100) | |||
| // searchBarBottomPadding's animateDpAsState has a finishedListener, how do I propagate | |||
There was a problem hiding this comment.
When you're faced with strange and complex logic like this, it usually means you need to rethink your approach to whatever you're doing in general
Of course, there are cases where complex logic is needed, but I don't think it's this :P
Why do we need to wait for animations to complete before navigating?
app/src/main/java/com/imashnake/animite/features/searchbar/SearchFrontDrop.kt
Outdated
Show resolved
Hide resolved
| @@ -177,9 +215,7 @@ fun ExpandedSearchBar(viewModel: SearchViewModel = viewModel()) { | |||
| IconButton( | |||
| onClick = { | |||
| text = "" | |||
There was a problem hiding this comment.
Does text not update based on VM state automatically? Maybe we can do something about that
| }, | ||
| modifier = Modifier | ||
| .align(Alignment.End) | ||
| .wrapContentSize(), | ||
| .wrapContentWidth() |
There was a problem hiding this comment.
Does wrapContentWidth make a difference here?
Unwrapped it in the parent.
Put it on top of the navigation bar.
Redundant scope function.
Made `modifier` optional.
boswelja
reviewed
Dec 5, 2022
| @@ -62,7 +62,7 @@ import com.imashnake.animite.R as Res | |||
| ) | |||
| @Composable | |||
| fun SearchFrontDrop( | |||
| modifier: Modifier, | |||
| modifier: Modifier = Modifier, | |||
There was a problem hiding this comment.
Default parameters should be last in the list, bring navController up (or remove it as per my last comment).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CONTRIBUTING.md.Padding is still a mess but this works better than the current implementation. As @boswelja suggested, we should probably use custom layouts to automatically apply padding.
Summary of changes:
1. This is only to test the newSearchcomponent, we probably won't merge this since padding inHomeis a goddamn mess, i.e., I concede to @boswelja's awfulScaffoldrefactor.2. Let's refactor to a
Scaffoldsomehow and bring in this new component.Searchsince it is not necessarily a "bar".imePadding().imeNestedScroll().Tested changes:
:heh: