Add option to disable top site suggestions #9063
Conversation
luixxiul
added
PR/work-in-progress ⚒
PR/needs-QA-attention ☕
help wanted
|
Help wanted:
Actual result: Expected result: nothing should be autocompleted. |
app/common/lib/suggestion.js
Outdated
| @@ -540,12 +540,15 @@ const getSearchSuggestions = (state, tabId, urlLocationLower) => { | |||
|
|
|||
| const getAlexaSuggestions = (state, urlLocationLower) => { | |||
| return new Promise((resolve, reject) => { | |||
| const options = { | |||
There was a problem hiding this comment.
Please follow the same pattern as here:
https://github.com/luixxiul/browser-laptop/blob/dd9ad5fe6077165d5d15df2428c5770a235f04d1/app/common/lib/suggestion.js#L504
Basically you resolve with an empty immutable list if the setting is not on.
|
We're past the freeze for 0.16.100 so moving this to 0.16.200. Only critical things and regressions are allowed past the freeze, thanks! |
luixxiul
added
PR/needs-QA-attention ☕
and removed
help wanted
| showHistoryMatches=Show history matches | ||
| showBookmarkMatches=Show bookmark matches | ||
| showOpenedTabMatches=Show tab matches | ||
| showTopsiteMatches=Show top site matches |
There was a problem hiding this comment.
while it follows this prefs pattern, wouldn't be better to say "show top site suggestions" instead?
There was a problem hiding this comment.
It looks reasonable. I'm addressing it.
There was a problem hiding this comment.
Addressed with 4844a15
Closes #4977 Auditors: Test Plan 1: 1. Open about:preferences#search 2. Disable 'Show top site matches' 3. Open a new tab 4. Input 'face' 5. Make sure that there does not appear the suggestions list Test Plan 2: 1. Open about:preferences#search 2. Enable 'Show top site matches' 3. Open a new tab 4. Input 'face' 5. Make sure there appears the suggestions list, which has facebook.com
|
@cezaraugusto the two commits were squashed and rebased. I'm going to replace the label with |
There was a problem hiding this comment.
changes look great, no regression in tests. Travis shows one suspicious test failing (ref: https://travis-ci.org/brave/browser-laptop/jobs/238833472#L3308) but I tested locally and it passes. Nice work ++
|
thanks! |
Closes #4977
Auditors:
Test Plan 1:
Show top site matchesfaceTest Plan 2:
Show top site matchesfaceSubmitter Checklist:
git rebase -ito squash commits (if needed).Reviewer Checklist:
Tests