From 8683703b7f8dc5448450f2a0dc95fae058e561d3 Mon Sep 17 00:00:00 2001 From: Ricky Rosario Date: Thu, 26 Jan 2017 14:38:21 -0500 Subject: [PATCH] fix (content): #2083 make large favicons fill the tile --- addon/Feeds/TopSitesFeed.js | 2 +- content-src/components/TopSites/TopSites.js | 21 +++++++--------- content-src/lib/first-run-data.js | 24 ++++++++++++++----- content-test/addon/Feeds/TopSitesFeed.test.js | 8 +++---- 4 files changed, 32 insertions(+), 23 deletions(-) diff --git a/addon/Feeds/TopSitesFeed.js b/addon/Feeds/TopSitesFeed.js index 96eece630c..3bee2e3c51 100644 --- a/addon/Feeds/TopSitesFeed.js +++ b/addon/Feeds/TopSitesFeed.js @@ -31,7 +31,7 @@ module.exports = class TopSitesFeed extends Feed { if (experiments.screenshots) { try { links = yield getScreenshots(links, site => { - if (site.favicon_height >= 64 && site.favicon_width >= 64 && isRootDomain(site.url)) { + if (site.favicon_height >= 96 && site.favicon_width >= 96 && isRootDomain(site.url)) { // If we are at the "root domain path" and the icon is big enough, // we don't show a screenshot. return false; diff --git a/content-src/components/TopSites/TopSites.js b/content-src/components/TopSites/TopSites.js index a945ec9502..6919a608e8 100644 --- a/content-src/components/TopSites/TopSites.js +++ b/content-src/components/TopSites/TopSites.js @@ -25,20 +25,15 @@ const TopSitesItem = React.createClass({ _faviconSize(site, topSitesExperimentIsOn, screenshot) { let faviconSize = 32; if (topSitesExperimentIsOn && !screenshot) { - if (!site.favicon_width) { - // default to 64 if not specified + if (site.favicon_url && (site.favicon_url.startsWith("favicons/images") || site.favicon_url.startsWith("resource://"))) { + // If it starts with favicons/images or resource:// then it's a tippy top icon. + // We want the size set to 64 for those. + // FIXME: long term we want the metadata parser to pass along where the image came from. faviconSize = 64; } else { - faviconSize = site.favicon_width; - } - - // We want the favicon to be at least 32x32 and at most 64x64 for now because - // I noticed a bunch of issues when letting the icons fill the tile (96x96). - // And we don't want them to be smaller than 32, our previous fixed size. - if (faviconSize > 64) { - faviconSize = 64; - } else if (faviconSize < 32) { - faviconSize = 32; + // If we have a normal (non tippy top) favicon, we're going to stretch + // or shrink it to be wall to wall. + faviconSize = 96; } } return faviconSize; @@ -51,6 +46,7 @@ const TopSitesItem = React.createClass({ const topSitesExperimentIsOn = this.props.showNewStyle; const screenshot = topSitesExperimentIsOn && site.screenshot; const faviconSize = this._faviconSize(site, topSitesExperimentIsOn, screenshot); + const showBackground = faviconSize < 96; // The top-corner class puts the site icon in the top corner, overlayed over the screenshot. const siteIconClasses = classNames("tile-img-container", {"top-corner": screenshot}); @@ -66,6 +62,7 @@ const TopSitesItem = React.createClass({ className={siteIconClasses} site={site} faviconSize={faviconSize} showTitle={!screenshot} + showBackground={showBackground} showNewStyle={topSitesExperimentIsOn} /> {screenshot &&
{label}
} diff --git a/content-src/lib/first-run-data.js b/content-src/lib/first-run-data.js index 8c47386ade..1d890c10b3 100644 --- a/content-src/lib/first-run-data.js +++ b/content-src/lib/first-run-data.js @@ -9,37 +9,49 @@ module.exports = { "title": "Facebook", "url": "https://www.facebook.com/", "favicon_url": "facebook-com.png", - "background_color": [59, 89, 152] + "background_color": [59, 89, 152], + "favicon_height": 64, + "favicon_width": 64 }, { "title": "YouTube", "url": "https://www.youtube.com/", "favicon_url": "youtube-com.png", - "background_color": [219, 67, 56] + "background_color": [219, 67, 56], + "favicon_height": 64, + "favicon_width": 64 }, { "title": "Amazon", "url": "http://www.amazon.com/", "favicon_url": "amazon-com.png", - "background_color": [255, 255, 255] + "background_color": [255, 255, 255], + "favicon_height": 64, + "favicon_width": 64 }, { "title": "Yahoo", "url": "https://www.yahoo.com/", "favicon_url": "yahoo-com.png", - "background_color": [80, 9, 167] + "background_color": [80, 9, 167], + "favicon_height": 64, + "favicon_width": 64 }, { "title": "eBay", "url": "http://www.ebay.com", "favicon_url": "ebay-com.png", - "background_color": [237, 237, 237] + "background_color": [237, 237, 237], + "favicon_height": 64, + "favicon_width": 64 }, { "title": "Twitter", "url": "https://twitter.com/", "favicon_url": "twitter-com.png", - "background_color": [4, 159, 245] + "background_color": [4, 159, 245], + "favicon_height": 64, + "favicon_width": 64 } ].map(item => { item.type = FIRST_RUN_TYPE; diff --git a/content-test/addon/Feeds/TopSitesFeed.test.js b/content-test/addon/Feeds/TopSitesFeed.test.js index 344bed43d0..7bf8b17a75 100644 --- a/content-test/addon/Feeds/TopSitesFeed.test.js +++ b/content-test/addon/Feeds/TopSitesFeed.test.js @@ -72,19 +72,19 @@ describe("TopSitesFeed", () => { const shouldGetScreenshot = getScreenshots.getCall(0).args[1]; assert.equal( false, - shouldGetScreenshot({favicon_width: 64, favicon_height: 64, url: "https://mozilla.org/"} + shouldGetScreenshot({favicon_width: 96, favicon_height: 96, url: "https://mozilla.org/"} )); assert.equal( false, - shouldGetScreenshot({favicon_width: 64, favicon_height: 64, url: "https://mozilla.org"} + shouldGetScreenshot({favicon_width: 96, favicon_height: 96, url: "https://mozilla.org"} )); assert.equal( true, - shouldGetScreenshot({favicon_width: 64, favicon_height: 64, url: "https://mozilla.org/foo"} + shouldGetScreenshot({favicon_width: 96, favicon_height: 96, url: "https://mozilla.org/foo"} )); assert.equal( true, - shouldGetScreenshot({favicon_width: 32, favicon_height: 32, url: "https://mozilla.org"} + shouldGetScreenshot({favicon_width: 95, favicon_height: 95, url: "https://mozilla.org"} )); }); });