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

fix topsites never returning after exclusion #12806

Merged
merged 1 commit into from
Jan 23, 2018
Merged

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jan 23, 2018

cc @LaurenWags

fix #10411

STR:

  1. Exclude a top site
  2. With 5 tiles only, visit a new website
  3. Website tile should be shown with a total of 6 tiles in newtab page

Please note that we debounce all newtab page data so the sixth tile will be shown only after a new site visit, even if you have other sites on the list.

@cezaraugusto cezaraugusto added this to the 0.20.x (Beta Channel) milestone Jan 23, 2018
@cezaraugusto cezaraugusto self-assigned this Jan 23, 2018
@@ -85,6 +89,7 @@ const getTopSiteData = () => {
let sites = historyState.getSites(state)
.filter((site, key) => !isSourceAboutUrl(site.get('location')) &&
!isPinned(state, key) &&
!isIgnored(state, site.get('key')) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use just key here same as for isPinned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok done

Copy link
Member

@LaurenWags LaurenWags left a comment

Choose a reason for hiding this comment

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

Assuming a clean profile, if I use the STR from the Test Plan Description I don't get a 6th tile.

If I follow the STR from #10411 (comment) I do get a 6th tile.

bsclifton
bsclifton previously approved these changes Jan 23, 2018
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.

Test plan works for me. It does take a second to show the new tile. It seems that I have to also have to switch to that tab in order for the icon to load (which then creates the tile). ++

@codecov-io
Copy link

codecov-io commented Jan 23, 2018

Codecov Report

Merging #12806 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #12806      +/-   ##
==========================================
+ Coverage   56.09%   56.11%   +0.01%     
==========================================
  Files         279      279              
  Lines       27309    27314       +5     
  Branches     4444     4445       +1     
==========================================
+ Hits        15319    15326       +7     
+ Misses      11990    11988       -2
Flag Coverage Δ
#unittest 56.11% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
app/browser/api/topSites.js 94.54% <100%> (+0.25%) ⬆️
app/common/state/aboutNewTabState.js 100% <0%> (+5.12%) ⬆️

Copy link
Member

@LaurenWags LaurenWags left a comment

Choose a reason for hiding this comment

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

both scenarios tried in #12806 (review) work as expected :)

@bsclifton bsclifton merged commit 4e6cbdd into master Jan 23, 2018
@bsclifton bsclifton deleted the topsites/excluded/10411 branch January 23, 2018 21:17
bsclifton added a commit that referenced this pull request Jan 23, 2018
fix topsites never returning after exclusion
bsclifton added a commit that referenced this pull request Jan 23, 2018
fix topsites never returning after exclusion
@bsclifton
Copy link
Member

master 4e6cbdd
0.21.x 57abcd3
0.20.x 0868041

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

Successfully merging this pull request may close these issues.

Removing top site tiles is buggy
5 participants