-
Notifications
You must be signed in to change notification settings - Fork 901
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
Address the tasks in the Meta section #5105
Address the tasks in the Meta section #5105
Conversation
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.
Added a few comments, but works as expected! 🚀
.onErrorReturn { emptyList() } | ||
.toObservable() | ||
// TODO: ANA - Do we want to have this check here, or somewhere else? (note: this is using the RxComputationThreadPool). | ||
if (autocompleteTabsFeature.self().isEnabled()) { |
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.
I think it's fine to keep it here, but I'm wondering whether we should cache the value, so we don't need to process the toggle every time. See HistoryFeature
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.
I updated this in the last PR -> https://github.com/duckduckgo/Android/pull/5123/files#diff-e563a062fe06ca0a38b0c058df8314a24f2fffb40952a85a1e356be04697b58bR264
@@ -247,13 +249,18 @@ class AutoCompleteApi @Inject constructor( | |||
} | |||
|
|||
private fun getAutocompleteSwitchToTabResults(query: String): Observable<MutableList<RankedSuggestion<AutoCompleteSwitchToTabSuggestion>>> = |
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.
nit: Can the return type have List
instead of MutableList
?
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.
All MutableList
were removed after moving away from rxJava -> #5123
.onErrorReturn { emptyList() } | ||
.toObservable() | ||
} else { | ||
Observable.just(mutableListOf()) |
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.
nit: Same here, can we just return listOf
?
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.
Can you add a few tests here?
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.
Added 👍
@@ -262,7 +260,7 @@ sealed class AutoCompleteViewHolder(itemView: View) : RecyclerView.ViewHolder(it | |||
editableSearchClickListener: (AutoCompleteSuggestion) -> Unit, | |||
omnibarPosition: OmnibarPosition, | |||
) = with(binding) { | |||
phrase.text = item.phrase | |||
phrase.text = item.phrase.formatIfUrl() |
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.
Can we move this one level up so we don't need to format phrase
for every type of element?
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.
Done in the final PR -> #5123
1ee6d24
to
12256db
Compare
418720f
to
cabd77b
Compare
c46cf24
to
5d0ed05
Compare
cabd77b
to
16012e8
Compare
5d0ed05
to
871743c
Compare
16012e8
to
ed1fee2
Compare
e62d927
to
26275f2
Compare
ed1fee2
to
f679db7
Compare
26275f2
to
838a6b3
Compare
4b8c8a2
to
e5b9a08
Compare
e65d5a4
to
f7a8db6
Compare
e5b9a08
to
bb5d02d
Compare
Task/Issue URL: https://app.asana.com/0/1200581511062568/1208040472309277/f ### Description Removed rxjava from autosuggest. ### Steps to test this PR Smoke test the autosuggest. There should be no user facing changes. This is the final PR. See scenarios and screenshots in https://app.asana.com/0/1200581511062568/1208264037618741/f ### NO UI changes
a62cc5a
into
feature/ana/add_translations_for_open_tabs_in_search_autosuggest_l10n
Task/Issue URL: https://app.asana.com/0/72649045549333/1208287265878043/f
Description
Added feature flag for switch to tab. Removed unneeded quick insert icons. Formatted urls in autosuggest. Added dividers between autosuggest sections.
Updated color on switch to tab icon.
Steps to test this PR
Feature 1
http://www.
,htps://www.
,www.
as prefix and they don't have a trailing/
.autocompleteTabs
fromFeature Flag Inventory
you don't see the Switch to tab suggestions.UI changes