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

Update top sites to only show 1 site per domain (for 0.12.9) #5565

Closed
bsclifton opened this issue Nov 11, 2016 · 0 comments
Closed

Update top sites to only show 1 site per domain (for 0.12.9) #5565

bsclifton opened this issue Nov 11, 2016 · 0 comments
Assignees
@bsclifton
Labels
feature/newtab QA/checked-macOS
Milestone
0.12.9dev

Comments

@bsclifton
Copy link
Member

Did you search for similar issues before submitting this one?
Yes

Describe the issue you encountered:
Currently, with 0.12.9 (dev-channel), you'll see up to 6 icons in the top sites area of about:newtab. For the 0.12.9 release, we need to ensure there is only one per domain.

A long term fix will be developed with #5338, but we need a simple rev1 before this new feature can be released.

Expected behavior:
Only one link per domain should show.

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    all

  • Brave Version:
    0.12.9

  • Steps to reproduce:

    1. Launch Brave and open a new tab
    2. open another tab and visit two pages on the same website
    3. Switch back to the new tab and notice there are two entries (which have the same favicon)

  • Screenshot if needed:

  • Any related issues:
    about:newtabs - have relevant results shown (implement frecency) #5338
@bsclifton bsclifton added this to the 0.12.9dev milestone Nov 11, 2016
@bsclifton bsclifton self-assigned this Nov 11, 2016
This was referenced Nov 13, 2016
Merged
Merged
bbondy pushed a commit that referenced this issue Nov 14, 2016
@bsclifton @bbondy
Refactoring, Bug fixes, and tests for top sites on about:newtab
ec3342c 
Bookmarking items does not affect position
Fixes #5413

Sites are ordered by most visited (count) DESC
Fixes #5322

Groundwork laid for #5565,
but the task is unfinished as-is.

-----
These commits moving existing logic from newtab.js into the session helper.
Tests were then added and I manually tested each scenario and tried to make
sure tests cover those. Things which are not covered are marked with  TODO(bsclifton)

The important thing:
appState.about.newtab.sites no longer persists a separate copy of the sites array.
It instead uses the location/partion of items saved to lookup the real object from appState.sites.
-----

Auditors: @cezaraugusto, @bbondy
bbondy pushed a commit that referenced this issue Nov 14, 2016
@bsclifton @bbondy
about:newtab - Update the unpinned site list to only show one result …
4ae810b 
…per domain.

Fixes #5565

Auditors: @bbondy

UPDATE:
- Updated unique check from array => set
Auditors: @bbondy
This was referenced Nov 15, 2016
Closed
Closed
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bsclifton bsclifton

Labels
feature/newtab QA/checked-macOS
Projects
None yet
Milestone
0.12.9dev
Development

No branches or pull requests
2 participants
@alexwykoff @bsclifton