-
-
Notifications
You must be signed in to change notification settings - Fork 52
Introduce a SearchBar #801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
de7d39a to
5da5ec7
Compare
5da5ec7 to
6597cd3
Compare
Oowoosh0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good to me. Just a couple of smaller issues I found:
- When nothing matches the search term then the "Queue is Empty" Placeholder gets shown. Probably it would be better to display a placeholder here that reflects that the search didn't return any results.
- There are some performance issues if the queue is larger (couple hundred songs). The UI freezes for a bit on every character entered into the search bar. I guess it is unavoidable that filtering a longer list takes a bit, but ideally the UI should stay responsive.
- If I clear the queue while a search is applied and then add new songs that search is automatically applied again. Maybe it's better to reset the search filter when clearing the queue. But I'm not really sure which of the two would be the expected behaviour for most people.
|
@Oowoosh0 Pushed a couple commits to address review comments. I think performance should be better here since we've switched to ListView previously |
ryonakano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@ryonakano fixed! |

Fixes #794
Introduce a search bar widget since we are probably gonna need search in more places if we're going to implement the library.