Skip to content
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

refactor new tab top sites #2971

Closed
cezaraugusto opened this issue Jan 15, 2019 · 4 comments · Fixed by brave/brave-core#4325 or brave/brave-core#5097
Closed

refactor new tab top sites #2971

cezaraugusto opened this issue Jan 15, 2019 · 4 comments · Fixed by brave/brave-core#4325 or brave/brave-core#5097

Comments

@cezaraugusto
Copy link
Contributor

cezaraugusto commented Jan 15, 2019

  • Improve drag n drop;
  • Simplify top sites logic;
  • Fix bad re-ordering while pinning -- sometimes pinning a site makes it jump to the wrong position;
  • Fix bookmarking top sites not showing the correct state.

Test Plan:

Only if you have an existing profile

  1. Other areas in NTP such as stats and rewards should not be affected by this change
  2. Your current top sites should still be there

Actual test plan

  1. New sites visited should be added first in the tiles grid. Note that timing for that may vary.
  2. You should be able to re-order tiles by drag and drop
  3. Tiles re-ordered should persist position upon browser refresh
  4. Pinned tiles can not be moved. Attempts to move sibling tiles should not change the pinned tile position
  5. Pinned tiles can not be excluded
  6. Tiles excluded should pop up a notification bar on top of the page asking if you want to un-do the operation
  7. Un-do a site removal should show back the tile in the first position or the first position next to the pinned site
  8. Bookmarking a tile should add a bookmark in the bookmark toolbar. Toggling it should remove the bookmark
@cezaraugusto cezaraugusto self-assigned this Jan 15, 2019
@cezaraugusto cezaraugusto added this to the 1.x Backlog milestone Jan 15, 2019
@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label Feb 6, 2019
@rebron rebron removed this from the 1.x Backlog milestone Feb 7, 2019
@cezaraugusto cezaraugusto assigned imptrx and unassigned cezaraugusto May 28, 2019
@cezaraugusto cezaraugusto assigned cezaraugusto and unassigned imptrx Aug 7, 2019
@Brave-Matt
Copy link

I have several Community users reporting that top-sites aren't functioning correctly:
https://community.brave.com/t/cant-remove-some-top-sites-on-brave/92194/26

cezaraugusto added a commit to brave/brave-core that referenced this issue Jan 8, 2020
cezaraugusto added a commit to brave/brave-core that referenced this issue Jan 16, 2020
bsclifton pushed a commit to brave/brave-core that referenced this issue Jan 17, 2020
cezaraugusto added a commit to brave/brave-core that referenced this issue Feb 8, 2020
cezaraugusto added a commit to brave/brave-core that referenced this issue Feb 17, 2020
@bsclifton
Copy link
Member

Reverted brave/brave-core#4325 with brave/brave-core#4996 because pinned items were lost
(see #8781)

Per @cezaraugusto:

  1. pinned sites were lost because I split storage data in NTP to avoid bugs in unrelated fields such as rewards. The fix for this is to grab current pinned sites data and migrate to the new storage
  2. the site removal bug Rafael mentioned will be addressed by changing the field name. This is the only part where I kept the same. This is a strong guess since I'm not able to repro

@bsclifton
Copy link
Member

Potential fix can also include fix for brave/brave-core#4987

@LaurenWags
Copy link
Member

LaurenWags commented Apr 28, 2020

Verified passed with

Brave 1.8.85 Chromium: 81.0.4044.122 (Official Build) (64-bit)
Revision 44f4233f08910d83b146130c1938256a2e05b136-refs/branch-heads/4044@{#963}
OS macOS Version 10.14.6 (Build 18G3020)

Clean profile:

  • Confirmed top site tiles are populated
    • Confirmed new sites are added first in the grid
  • Confirmed able to delete tiles
    • Confirmed able to "Undo" delete of a single or all tiles.
  • Confirmed able to drag and drop to reorder tiles
    • Confirmed reorder is persisted on browser re-launch
  • Confirmed able to pin/unpin tiles
    • Confirmed unable to drag/drop pinned tiles
    • Confirmed unable to exclude pinned tiles
  • Confirmed able to bookmark/unbookmark sites from tiles

Upgrade profile:

  • Confirmed top site tiles still populated as expected
  • Confirmed tiles still in pinned positions as expected
  • Issue with bookmark icon (noted below)
  • Confirmed other areas (clock, stats, BR widget) are unaffected
  • Confirmed able to delete a tile, but issue when tile delete is "Undone" (noted below)
  • Pinned sites not duplicated initially, but do get duplicated in a few scenarios (fixed with issue noted below)
  • Confirmed able to bookmark/unbookmark sites from tiles on upgraded profile
  • Confirmed able to drag and drop to reorder tiles on upgraded profile
    • Confirmed reorder is persisted on browser re-launch for upgraded profile
  • Confirmed as I visited sites on the upgraded profile new sites were added to the tiles
    • Confirmed new sites are added first in the grid on upgrade profile
  • Confirmed able to pin/unpin tiles on upgraded profile
    • Confirmed unable to drag/drop pinned tiles on upgraded profile
    • Confirmed unable to exclude pinned tiles on upgraded profile

1.7.98:
1 7 98

1.8.85:
1 8 85

Verification passed on

Brave 1.8.85 Chromium: 81.0.4044.122 (Official Build) (64-bit)
Revision 44f4233f08910d83b146130c1938256a2e05b136-refs/branch-heads/4044@{#963}
OS Windows 10 OS Version 1803 (Build 17134.1006)

Clean profile:

  • Confirmed top site tiles are populated
    • Confirmed new sites are added first in the grid
  • Confirmed able to delete tiles
    • Confirmed able to "Undo" delete of a single or all tiles.
  • Confirmed able to drag and drop to reorder tiles
    • Confirmed reorder is persisted on browser re-launch
  • Confirmed able to pin/unpin tiles
    • Confirmed unable to drag/drop pinned tiles
    • Confirmed unable to exclude pinned tiles
  • Confirmed able to bookmark/unbookmark sites from tiles

Upgrade profile:

Verification passed on

Brave 1.8.85 Chromium: 81.0.4044.122 (Official Build) (64-bit)
Revision 44f4233f08910d83b146130c1938256a2e05b136-refs/branch-heads/4044@{#963}
OS Ubuntu 18.04 LTS

Clean profile:

  • Confirmed top site tiles are populated
    • Confirmed new sites are added first in the grid
  • Confirmed able to delete tiles
    • Confirmed able to "Undo" delete of a single or all tiles.
  • Confirmed able to drag and drop to reorder tiles
    • Confirmed reorder is persisted on browser re-launch
  • Confirmed able to pin/unpin tiles
    • Confirmed unable to drag/drop pinned tiles
    • Confirmed unable to exclude pinned tiles
  • Confirmed able to bookmark/unbookmark sites from tiles

Upgrade profile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment