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 DnD on NTP Top Sites #4325

Merged
merged 5 commits into from
Mar 19, 2020
Merged

Refactor DnD on NTP Top Sites #4325

merged 5 commits into from
Mar 19, 2020

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jan 8, 2020

Close brave/brave-browser#2971

Submitter Checklist:

Test Plan:

Assertions:

  • Fresh profile should have 0 entries
  • Non-fresh profile should show your current top sites unpinned
  • Should be able to drag n' drop tiles without errors
  • Should not be able to drag a pinned tile
  • Should not be able to drop over a pinned tile
  • Changes should persist on page refresh
  • Should be able to exclude a site
  • Should be able to undo an excluded site, which should go back to the first grid position
  • Should be able to undo all excluded sites, which should go back to first grid positions
  • Should be able to spot a bookmarked site on first render
  • Should be able to toggle a bookmarked site

Manual test plan:

Note: Chromium topSites API is not reliable and can drastically vary the time to update the list. Below steps will be async so it's not guaranteed that you will se the site after you visit it, but they should work after some site interaction.

  1. Fresh profile
  2. Visit https://brave.com
  3. Visit https://quora.com
  4. Once sites are available on the grid, pin the site on the 2nd grid position
  5. Visit https://twitter.com
  6. Pinned site should retain position: grid should be Twitter (1), PINNED_SITE (2), other site (3)

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@cezaraugusto cezaraugusto added feature/newtab CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Jan 8, 2020
@cezaraugusto cezaraugusto self-assigned this Jan 8, 2020
@bsclifton
Copy link
Member

bsclifton commented Jan 8, 2020

Built locally - a few things I noticed:

  1. dragging a pinned item still let's you drag into the omnibox. I think this is OK- but just mentioning because it will show the ghost icon for the favicon as you are dragging. The tile itself doesn't move
  2. you can drag a tile OVER a pinned tile and it appears to move. However, when you drop (release mouse), it goes back to where it was. Is this expected?
  3. the bookmark button on the tiles doesn't seem to be working at all
  4. If I click the X or bookmark button on the first tile (I only have two tiles), it causes the second to be pinned?

I'm going to re-run sync / build and clear my profile directory again. Something must be wrong on my side

@bsclifton
Copy link
Member

OK did a rebuild, issue 4 went away, but all the rest still exist

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@cezaraugusto
Copy link
Contributor Author

thanks for checking, I'm addressing your comments. I've run into a race condition and topSites API isn't working reliably which makes testing hard.

re your comments:

  1. yes. link should work just can't re-order the tile;
  2. yes, expected;
  3. bookmarks doesn't work for me on latest build and it's been this way for a while. In this PR I removed the bookmark code to avoid increasing the number of changes. Plan was to make it as a follow-up but can be added if we feel so.

@cezaraugusto cezaraugusto force-pushed the ca-2971 branch 2 times, most recently from 41ed8c0 to b8adc20 Compare January 16, 2020 02:41
@cezaraugusto
Copy link
Contributor Author

PR ready for re-review

@cezaraugusto
Copy link
Contributor Author

also test plan updated

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cezaraugusto cezaraugusto force-pushed the ca-2971 branch 3 times, most recently from 6d5c279 to df4f1a8 Compare February 8, 2020 18:20
@cezaraugusto
Copy link
Contributor Author

Added several key tests and rebranded "topSites" with "gridSites". Since we plan to keep Chromium-only top sites as an option in the near future, it helps that both have different names.

Travis is failing due to Storybook. There is a separate PR to fix it once for all: #4470.

@cezaraugusto cezaraugusto force-pushed the ca-2971 branch 4 times, most recently from e358a4f to afd10dd Compare March 18, 2020 21:21
@cezaraugusto cezaraugusto force-pushed the ca-2971 branch 2 times, most recently from 7131e0a to 126882f Compare March 19, 2020 16:53
@cezaraugusto
Copy link
Contributor Author

Going to merge this one as it's been sitting here for while even after being approved. Please ping in case this causes any harm and I'll revert/revisit. Thanks!

@cezaraugusto cezaraugusto merged commit 3203693 into master Mar 19, 2020
@cezaraugusto cezaraugusto deleted the ca-2971 branch March 19, 2020 17:25
@cezaraugusto cezaraugusto added this to the 1.8.x - Nightly milestone Mar 19, 2020
bsclifton added a commit that referenced this pull request Mar 20, 2020
This reverts commit 3203693, reversing
changes made to 80efc08.
bsclifton added a commit that referenced this pull request Mar 20, 2020
Revert "Merge pull request #4325 from brave/ca-2971"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS feature/newtab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor new tab top sites
2 participants