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

Create default topSites for new tab page #5623

Merged
merged 1 commit into from
Nov 21, 2016
Merged

Create default topSites for new tab page #5623

merged 1 commit into from
Nov 21, 2016

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Nov 15, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fix #5594

Auditors: @bsclifton

@bradleyrichter:

  • I edited topSites icon default size, from 32px to 64px. It affects not only default topSites but any other site. Can be changed as needed;
  • Default sites' icon are now stored within Brave. I used arbitrary icons, can also be changed as needed.

Test Plan:

  1. On a fresh (clean session-store) view, new tab should have a default grid with 6 sites,
    being them described on referenced issue
  2. Visiting a new site should override previous grid
  3. If session-store is previously populated, default topSites shouldn't appear on grid

Note: Since we're having some issues with accessed sites, it may be required to refresh the new site in order for (2) to work

@bbondy
Copy link
Member

bbondy commented Nov 15, 2016

This needs to be rebased please

@bbondy
Copy link
Member

bbondy commented Nov 15, 2016

@bradleyrichter did we decide on these top sites?
I would put the iOS app instead of youtube

@bbondy
Copy link
Member

bbondy commented Nov 15, 2016

If we are going with these I think we should cache these tile icons as well so we don't send requests to these places without a user visiting them. @bradleyrichter ?

@bradleyrichter
Copy link
Contributor

@bbondy and we could use larger favicons so the initial view is crispy...

@bradleyrichter
Copy link
Contributor

I think youtube is going to get more traffic than facebook but I will ask Catherine.

@bradleyrichter
Copy link
Contributor

Catherine suggested to swap Git for iOS app store, and this matches BE's opinion.

@bsclifton
Copy link
Member

bsclifton commented Nov 17, 2016

Change looks good- only concern I have (@bbondy, @bradleyrichter) is this. We could show the default sites in two ways:

  1. Only show the default sites if you have 0 sites. As soon as you have 1, they all disappear.
  2. Show the 6 default sites and then have the user's sites take priority over them. ex: if the user has only 2 top sites showing, grab 4 of the default ones.

This PR is doing option 1... is that what we'd like? I personally would prefer option 2, but I'm curious what you all think 😄

Besides that, all that's left is the feedback from @bbondy; we can likely put a high res copy of the favicon into app/extensions/brave/img/. (I wouldn't say this is blocking- but a user did reach out to support, asking why does Brave make requests for these)

Here's what it looks like right now; notice the default sites have a larger favicon size (64px x 64px):
screen shot 2016-11-16 at 5 19 25 pm

@bradleyrichter
Copy link
Contributor

please change to #2.

And when we have freceny done, the defaults will stay around longer and not be replaced even before a user sees them for the second time

@garvankeeley may have comments related to his improvements over the same issues with iOS top sites.

@bbondy
Copy link
Member

bbondy commented Nov 17, 2016

Still needs to load icons not remotely, moving this to 0.12.11

@bbondy bbondy modified the milestones: 0.12.11, 0.12.10 release Nov 17, 2016
@cezaraugusto
Copy link
Contributor Author

PR updated.

@bbondy sites' icon are now stored within the app;
@bradleyrichter I updated this PR resume with info about design changes I did. All open for new changes per request.
@bsclifton code ready for review

* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const fs = require('fs')
Copy link
Member

Choose a reason for hiding this comment

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

This is never used, can you remove it pls? 😄

Fix #5594

Auditors: @bsclifton

/cc @bradleyrichter

Test Plan:
* On a fresh (clean session-store) view, new tab should have a default grid with 6 sites,
being them described on referenced issue
* Visiting a new site should override previous grid
* If session-store is previously populated, default topSites shouldn't appear on grid
@cezaraugusto
Copy link
Contributor Author

@bsclifton thanks, updated

@bsclifton
Copy link
Member

Looks good to me 😄

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.

5 participants