Conversation
ghost
assigned
sashei
force-pushed
the
467-search-ui
branch
2 times, most recently
from
2fe47a6 to
d156d0f
Compare
jhugman
left a comment
jhugman
left a comment
There was a problem hiding this comment.
A few nits, but looking good!
| (type as? ItemListAdapterType.AutofillFilter)?.let { | ||
| if (count == 0 && !it.textEntered) | ||
| return 0 | ||
| } |
|
|
||
| if (type is ItemListAdapterType.AutofillFilter) { | ||
| view.disclosureIndicator.visibility = GONE | ||
| } |
| .addTo(compositeDisposable) | ||
| } | ||
|
|
||
| private fun Observable<Pair<CharSequence, List<ItemViewModel>>>.filterItemsForText(): Observable<Pair<CharSequence, List<ItemViewModel>>> { |
There was a problem hiding this comment.
:headsplode: I didn't know you could monkey patch from within a class.
| fun updateItems(newItems: List<ItemViewModel>, isFiltering: Boolean = false) { | ||
| this.isFiltering = isFiltering | ||
| fun updateItems(newItems: List<ItemViewModel>, type: ItemListAdapterType = ItemListAdapterType.ItemList) { | ||
| this.type = type |
There was a problem hiding this comment.
How often does this.type change. I think I would prefer this as an instance variable passed in to the adapter's constructor.
| ) | ||
| val displayNoEntries = it.second.isEmpty() || !it.first.isEmpty() | ||
| val itemList = if (it.first.isEmpty()) emptyList() else it.second | ||
| Pair(itemList, displayNoEntries) |
There was a problem hiding this comment.
Nit:
itemList to displayNoEntries| Pair(pair.first, pair.second.filter { | ||
| it.title.contains(pair.first, true) || | ||
| it.subtitle.contains(pair.first, true) | ||
| }) |
There was a problem hiding this comment.
Nit: Pair(pair.pair, pair.filter { it.contains pair }
This feels like it could be clearer.
return this.map { (text, list) ->
val filtered = list.filter { it.title.contains(text, true) || it.subtitle.contains(text, true) }
text to filtered
}There was a problem hiding this comment.
Also: I'm not sure why this needs its own extension method.
There was a problem hiding this comment.
Also: consider adding a contains method to ItemViewModel.
| val cancelButtonClicks: Observable<Unit> | ||
| val cancelButtonVisibility: Consumer<in Boolean> | ||
| val itemSelection: Observable<ItemViewModel> | ||
| fun updateItems(items: List<ItemViewModel>, displayNoEntries: Boolean) |
There was a problem hiding this comment.
I'm not wild about this method call. Either call updateItems or displayNoEntries. Currently the method is doing double duty here.
| return@map AutofillAction.Complete(serverPassword) | ||
| } | ||
|
|
||
| AutofillAction.Cancel |
There was a problem hiding this comment.
Nit
it.value?.let { serverPassword ->
AutofillAction.Complete(serverPassword)
} ?: AutofillAction.Cancel
jhugman
left a comment
jhugman
left a comment
There was a problem hiding this comment.
Looking good.
I think we should file an issue to rationalize the FilterPresenter and AutofillFilterPresenter, but I'm really liking how this has shaped up.
| filteredItems | ||
| .map { !it.first.isEmpty() || it.second.isEmpty() } | ||
| .subscribe(view::displayNoEntries) | ||
| .addTo(compositeDisposable) |
| Pair(pair.first, pair.second.filter { | ||
| it.title.contains(pair.first, true) || | ||
| it.subtitle.contains(pair.first, true) | ||
| }) |
There was a problem hiding this comment.
Nit: make an ItemViewModel.contains(string: CharSequence) method.
.map { (text, list) ->
text to list.filter { it.contains(text) }
}(just noticed that this is also in FilterPresenter)
|
|
||
| class ItemListAdapter( | ||
| val type: ItemListAdapterType | ||
| ) : RecyclerView.Adapter<ItemListCell>() { |
| (type as? ItemListAdapterType.AutofillFilter)?.let { | ||
| if (count == 0 && !it.textEntered) | ||
| return 0 | ||
| } |
2 participants
Fixes #467
Testing and Review Notes
I couldn't find the designs for "No entries found" in Zeplin so I just took a shot in the dark at what the expected text qualities are.
I also made a design compromise in putting together the list view; when there are more than ~2 entries found, the dialog will touch to the top of the keyboard rather than "floating" as it does in the designs. Hopefully that's acceptable!!
To Do