Remove Brave software links from urlbar suggestions #7692
Conversation
There was a problem hiding this comment.
Just tested it out- I personally think this is a great solution. There is a problem though- as soon as you visit a site (any site), it wipes all the bookmarks from the new tab page. Even if you click one of the links on the new tab page
@bradleyrichter is this acceptable?
If we try to keep the bookmarks there, this means we'll have to put work-arounds in our code which are only used for these temporary bookmark entries (which isn't worth it, IMO). When someone launches, it's really nice because they can see what that area is for, even if it gets erased on their first website visit
|
Actually, I may have found a deal breaker (even if this behavior is OK) @diracdeltas you are allowed to pin the items... if you choose to pin any of the default bookmarks, it doesn't stick. In the above GIF, you can see that the Brave Twitter site is pinned and it gets erased after visiting the Facebook page |
|
@bsclifton great catch! i fixed the pinned-site-going-away-after-a-site-visit bug. personally i am okay with the new tab default sites going away as soon as you have a real site for the new tab page, but that is probably not the desired UX. |
fix #7655 Auditors: @bsclifton Test Plan: 1. open a clean instance of brave 2. type 'face' into the urlbar 3. it should autocomplete to facebook instead of Brave's facebook page
…inned sites. Auditors: @diracdeltas
There was a problem hiding this comment.
Added a follow up- the twitter site is no longer pinned by default (since, with these changes, it's not ACTUALLY pinned yet).
Changes look good 😄 I'll add @bradleyrichter as a reviewer too
|
going to merge this for now. @bradleyrichter if you have thoughts on persisting Brave sites on the newtab page, please open a follow-up issue. |
|
@bsclifton @diracdeltas doesn't this PR solve this issue and let us keep the default tiles as they were? |
|
@bradleyrichter as mentioned in the issue, i don't think so, because the Brave sites are still in the autosuggest bar. i think the OP wanted them to not be in there at all. |
|
We need full tiles there on first run please. The row-selector is coming back which allows the user to select 1/2/3 rows as well. |
|
they are in there on first run |
|
and vanish on the first visit to a new page? I think this will just look like a new bug. : ) |
|
i'm a fan of minimizing how much we self-advertise, but feel free to open an issue |
|
OK- I think we jumped the gun on this one. @bradleyrichter is going to work out the desired behavior. For now, we can leave as-is, but we'll want to change (and revisit) before release. I'll create an issue to track that (so that we don't stomp on the old issue(s)) edit: issue created #7717 |
Somehow this commit got lost from #7692 Auditors: @bsclifton @bradleyrichter Test Plan: 1. open new instance of brave 2. type facebook in urlbar 3. should autocomplete to facebook.com, not facebook.com/bravesoftware
(see #7717 for more info) This PR: - Reverts "Remove Brave software links from urlbar suggestions" (commit fa2eb80) - Reverts "Since new list is cleared after a visit, let's not have any default pinned sites." (commit 34e6be4) - Reverts "Actually remove Brave software links from history" (commit 3ef5da3) Auditors: @diracdeltas, @bradleyrichter
|
Removing milestone as this was reverted with e23e251 |
fix #7655
Auditors: @bsclifton
Test Plan:
git rebase -ito squash commits (if needed).Test Plan: