Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

hidden top site fixes you wouldn't believe existed #12470

Merged
merged 5 commits into from
Jan 8, 2018

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jan 3, 2018

address #12435
fix #12027
fix #12026
fix #11102
fix #5768
fix #5413

Test plan:

Please note that several topSites features are broken and tracked in #12435 (there's likely more issues). This PR only addresses what's covered by referenced issues.

Covered by npm run test -- --grep="respects position of pinned items when populating results"

Manual test plan (ensure you're on a fresh profile first):

#12027

visit some websites and ensure they're on the topsites list. Restart browser several times, sites shouldn't be missing

#12026

visit some websites and ensure they're on the topsites list. Restart. You shouldn't see default topSites anywhere else as previous position (they shouldn't appear momentarily)

#11102

visit some websites and ensure they're on the topsites list. Restart. Visit other sites. Top sites shouldn't be missing

#5768

visit some websites and ensure they're on the topsites list. pin 2nd and 4th websites. Visited site should be stored in the first position.

#5413

bookmark any default topsite using the star icon inside tile. Shouldn't jump position. Visit some websites and repeat operation. They shouldn't jump positions.

@cezaraugusto cezaraugusto added this to the 0.20.x (Beta Channel) milestone Jan 3, 2018
@cezaraugusto cezaraugusto self-assigned this Jan 3, 2018
@cezaraugusto cezaraugusto changed the title pinned sites should retain position hidden top site fixes you wouldn't believe existed Jan 3, 2018
@codecov-io
Copy link

codecov-io commented Jan 3, 2018

Codecov Report

Merging #12470 into master will decrease coverage by 0.02%.
The diff coverage is 64.58%.

@@            Coverage Diff             @@
##           master   #12470      +/-   ##
==========================================
- Coverage   55.91%   55.89%   -0.03%     
==========================================
  Files         278      278              
  Lines       26983    27015      +32     
  Branches     4363     4369       +6     
==========================================
+ Hits        15087    15099      +12     
- Misses      11896    11916      +20
Flag Coverage Δ
#unittest 55.89% <64.58%> (-0.03%) ⬇️
Impacted Files Coverage Δ
app/sessionStore.js 86.75% <ø> (-0.02%) ⬇️
app/browser/api/topSites.js 94.54% <100%> (+1.28%) ⬆️
app/common/state/aboutNewTabState.js 100% <100%> (ø) ⬆️
js/about/newtab.js 43.01% <5.55%> (-6.99%) ⬇️

-
default pinned top site was persistent even after restart. changes address it
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.

All issues as described are fixed. However, I did notice two issues (not sure if new? or regression? If you're able to repro, let's create a new issue)

  1. When pinning a site, it moves the position once (then stays)
  2. Bookmarking a top sites tile does not affect the star icon. It's supposed to fill it in. I tried closing/re-opening Brave and had no luck

@bsclifton bsclifton merged commit c71bea4 into master Jan 8, 2018
@bsclifton bsclifton deleted the topsites-revisited branch January 8, 2018 19:24
bsclifton added a commit that referenced this pull request Jan 8, 2018
hidden top site fixes you wouldn't believe existed
bsclifton added a commit that referenced this pull request Jan 8, 2018
hidden top site fixes you wouldn't believe existed
@bsclifton
Copy link
Member

master c71bea4
0.21.x df4597a
0.20.x 5e56a7c

@cezaraugusto
Copy link
Contributor Author

created #12554 for tile jump when pinned
and #11847 is already set for bookmark icon

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