Improve topsite tiles in NTP #7859

simonhong · 2021-02-08T07:48:15Z

fix brave/brave-browser#7493
fix brave/brave-browser#13881

Submitter Checklist:

There is a ticket for my issue.
Used Github auto-closing keywords in the commit message.
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed).
Requested a security/privacy review as needed.

Reviewer Checklist:

New files have MPL-2.0 license header.
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

In frequency mode, max tile number is 6 and add tile button is not displayed.
In favorites mode, 12 tiles are visible at most and add tile button is only visible when hover to top site tiles.
In favorites mode, add tile button is always visible if there is no tile.
Edit menu on tile always have two items - edit and delete.
When tile is edited or added, current mode is changed to favorites.

Full spec available at https://github.com/brave/internal/issues/754

bsclifton

Awesome work here! I'm sure this ended up being more JavaScript than you had expected 😄

Everything I saw looks great - would be good to solve the concern by @petemill RE: dark mode (so that it happens automatically, detected in media query). Thanks for following up on all the feedback @karenkliu had too - the text under the icons is a beautiful touch

simonhong · 2021-02-25T00:31:37Z

I found that it's switched to favorites mode but edited item is not visible in favorites mode. checking. => Fixed

Switch to Favorites
  The Top Sites widget switches to user-curated Favorites if the user manually customizes Top Sites:
      Edits a frequently visited site and puts in a site of their own choosing

simonhong · 2021-02-25T10:34:49Z

@karenkliu @petemill PTAL again.

bridiver · 2021-02-25T17:40:34Z

browser/ui/webui/new_tab_page/brave_new_tab_ui_utils.cc

+#include "url/gurl.h"
+
+std::string GetValidURLStringForTopSite(const std::string& url) {
+  return GURL(url).is_valid() ? url : "https://" + url;


This check is incomplete. You should re-verify that the url is valid after appending https

Added verification after fixing url here. If fixed url is not valid, that item will not be added/modified with passed url.

bridiver

deps/chromium_src ok

karenkliu · 2021-02-26T01:12:57Z

@simonhong did an amazing job fixing everything outlined in #7859 (review). The only thing we should do in a follow up is clean up top sites hover area to fix the the 'Add site' tile flicker. It sounds like it's a wrapping area issue that needs some refactoring.

simonhong · 2021-02-26T09:11:54Z

@simonhong did an amazing job fixing everything outlined in #7859 (review). The only thing we should do in a follow up is clean up top sites hover area to fix the the 'Add site' tile flicker. It sounds like it's a wrapping area issue that needs some refactoring.

@karenkliu Add button flickering is a little bit improved. Only flicked when hovered over menu button.

petemill

Thanks for addressing a lot of the feedback, this continues to work great. Still a couple of things, including an important accessibility issue, that we can fix in follow-up. We can keep a list here, if possible. Also is there any chance of squashing some of the commits - 40 seems a lot and many of them seem to be fixes that can be amended.

* Adjust max topsite tiles number to 12 * Can add/edit topsites from NTP * Can toggle favorites and frecency mode from NTP customization option fix brave/brave-browser#7493

simonhong · 2021-03-02T04:00:46Z

Merged because only audit-dps is failed from post-init and it's known issue.

Vantiveman · 2021-03-02T15:09:37Z

Is this released? I have the latest version and I don't have this yet. Perhaps not yet rolled out.......

simonhong · 2021-03-03T06:41:49Z

@Vantiveman This is available in nightly (1.23.x) now.

Vantiveman · 2021-03-03T13:14:24Z

Hi Simon, Thank you. How do I subscribe or download nightlies? From: Simon Hong <notifications@github.com> Sent: 03 March 2021 06:42 To: brave/brave-core <brave-core@noreply.github.com> Cc: Vantiveman <awilson@clara.net>; Mention <mention@noreply.github.com> Subject: Re: [brave/brave-core] Improve topsite tiles in NTP (#7859) @Vantiveman <https://github.com/Vantiveman> This is available in nightly (1.23.x) now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7859 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AK4UOK3TFP53KC732L5CWKLTBXK3XANCNFSM4XIPTOHQ> . <https://github.com/notifications/beacon/AK4UOK4GHQQF2LLZHWPNOETTBXK3XA5CNFSM4XIPTOH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOF4HH2UQ.gif>

simonhong · 2021-03-04T00:39:12Z

@Vantiveman Yes, https://brave.com/download-nightly/ is for you :)

MamaCoop452 · 2021-03-10T01:13:56Z

I'm truly confused. Sorry, I get how it should be able to be done, but that is not what I see when I open a new tab. Am I missing something? Are you really telling me that I need to redownload this every night in order to get these new updates?

simonhong · 2021-03-10T01:32:32Z

@MamaCoop452 I assume you're using brave stable channel. If so, this fix will be available for you when 1.23.x becomes stable version.
Currently, this is only available nightly channel (1.23.x). If you want to test this feature in advance, I recommend to install nightly(via https://brave.com/download-nightly).

simonhong self-assigned this Feb 8, 2021

simonhong force-pushed the topsite_edit branch 19 times, most recently from c7ec590 to 365270a Compare February 19, 2021 08:58

simonhong marked this pull request as ready for review February 19, 2021 09:35

simonhong requested a review from a team as a code owner February 19, 2021 09:35

simonhong force-pushed the topsite_edit branch from 365270a to 898ef54 Compare February 19, 2021 09:35

simonhong requested a review from bridiver as a code owner February 19, 2021 18:23

simonhong force-pushed the topsite_edit branch from cdd09cb to d5c1998 Compare February 20, 2021 02:33

simonhong changed the title ~~WIP: Improve topsite tiles in NTP~~ Improve topsite tiles in NTP Feb 20, 2021

simonhong requested review from bsclifton, petemill and zenparsing February 20, 2021 02:47

simonhong force-pushed the topsite_edit branch from 674eb7f to 74fa4ce Compare February 20, 2021 05:19

bsclifton approved these changes Feb 24, 2021

View reviewed changes

simonhong added CI/skip Do not run CI builds (except noplatform) and removed CI/skip Do not run CI builds (except noplatform) labels Feb 25, 2021

simonhong force-pushed the topsite_edit branch 2 times, most recently from f34c943 to 60320aa Compare February 25, 2021 08:09

bridiver reviewed Feb 25, 2021

View reviewed changes

bridiver approved these changes Feb 26, 2021

View reviewed changes

karenkliu approved these changes Feb 26, 2021

View reviewed changes

simonhong requested a review from petemill February 26, 2021 03:53

simonhong force-pushed the topsite_edit branch from 5a80c8c to 068cbb3 Compare February 26, 2021 11:17

petemill approved these changes Mar 1, 2021

View reviewed changes

simonhong force-pushed the topsite_edit branch from 068cbb3 to b47de61 Compare March 2, 2021 00:42

Improve topsites in NTP

5dc84a2

* Adjust max topsite tiles number to 12 * Can add/edit topsites from NTP * Can toggle favorites and frecency mode from NTP customization option fix brave/brave-browser#7493

simonhong force-pushed the topsite_edit branch from b47de61 to 5dc84a2 Compare March 2, 2021 00:54

simonhong merged commit 43ab1ab into master Mar 2, 2021

simonhong deleted the topsite_edit branch March 2, 2021 04:00

simonhong added this to the 1.23.x - Nightly milestone Mar 3, 2021

This was referenced Mar 3, 2021

Dashboard on new tab page should be customizable brave/brave-browser#353

Closed

Add ability to customize number of top sites shown on Dashboard (new tab page) brave/brave-browser#6619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve topsite tiles in NTP #7859

Improve topsite tiles in NTP #7859

simonhong commented Feb 8, 2021 •

edited

Loading

bsclifton left a comment

simonhong commented Feb 25, 2021 •

edited

Loading

simonhong commented Feb 25, 2021

bridiver Feb 25, 2021

simonhong Feb 26, 2021

bridiver left a comment

karenkliu commented Feb 26, 2021

simonhong commented Feb 26, 2021

petemill left a comment

simonhong commented Mar 2, 2021

Vantiveman commented Mar 2, 2021

simonhong commented Mar 3, 2021

Vantiveman commented Mar 3, 2021 via email

simonhong commented Mar 4, 2021

MamaCoop452 commented Mar 10, 2021

simonhong commented Mar 10, 2021

Improve topsite tiles in NTP #7859

Improve topsite tiles in NTP #7859

Conversation

simonhong commented Feb 8, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bsclifton left a comment

Choose a reason for hiding this comment

simonhong commented Feb 25, 2021 • edited Loading

simonhong commented Feb 25, 2021

bridiver Feb 25, 2021

Choose a reason for hiding this comment

simonhong Feb 26, 2021

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

karenkliu commented Feb 26, 2021

simonhong commented Feb 26, 2021

petemill left a comment

Choose a reason for hiding this comment

simonhong commented Mar 2, 2021

Vantiveman commented Mar 2, 2021

simonhong commented Mar 3, 2021

Vantiveman commented Mar 3, 2021 via email

simonhong commented Mar 4, 2021

MamaCoop452 commented Mar 10, 2021

simonhong commented Mar 10, 2021

simonhong commented Feb 8, 2021 •

edited

Loading

simonhong commented Feb 25, 2021 •

edited

Loading