feat (systemaddon): #2944 add tippy top icons #3049

rlr · 2017-08-01T02:38:08Z

coveralls · 2017-08-01T02:42:31Z

Coverage remained the same at 86.407% when pulling 202375f on rlr:gh2944/tippy-tops into c51a689 on mozilla:master.

rlr · 2017-08-01T02:44:43Z

We still need to cut a limited tippy top release but that can be done as a quick followup.

r? @Mardak and/or @jaredkerim and/or ?

Mardak

Looks like things are working fine. Just a bunch of various nits

Mardak · 2017-08-01T02:53:09Z

system-addon/content-src/components/TopSites/TopSites.jsx

+              }
+              {!showScreenshot &&
+                <div className="tippy-top-icon" style={style} />
+              }


This looks like it could just be maybe <div className={imageClassName} style={imageStyle} /> and unconditionally render that.

Or… we could be even fancier :p ?
<div {...imageAttributes} />

Mardak · 2017-08-01T02:55:00Z

system-addon/content-src/components/TopSites/TopSites.jsx

@@ -43,12 +43,24 @@ class TopSite extends React.Component {
    const title = link.pinTitle || shortURL(link);
    const screenshotClassName = `screenshot${link.screenshot ? " active" : ""}`;
    const topSiteOuterClassName = `top-site-outer${isContextMenuOpen ? " active" : ""}`;
-    const style = {backgroundImage: (link.screenshot ? `url(${link.screenshot})` : "none")};
+    const tippyTopIcon = link.tippyTopIcon;
+    const showScreenshot = !tippyTopIcon;


nit: just make this const {tippyTopIcon} = link;

Mardak · 2017-08-01T02:57:10Z

system-addon/content-src/components/TopSites/TopSites.jsx

-    const style = {backgroundImage: (link.screenshot ? `url(${link.screenshot})` : "none")};
+    const tippyTopIcon = link.tippyTopIcon;
+    const showScreenshot = !tippyTopIcon;
+    let style;


If we're going to have a imageClassName then let's make this imageStyle.

Mardak · 2017-08-01T02:58:40Z

system-addon/content-src/components/TopSites/TopSites.jsx

+    if (showScreenshot) {
+      style = {backgroundImage: (link.screenshot ? `url(${link.screenshot})` : "none")};
+    } else {
+      style = {backgroundImage: `url(${tippyTopIcon})`, backgroundColor: link.backgroundColor || "none"};


nit: let's split this latter object (with 2 keys) onto multiple lines. And just make the condition

if (tippyTopIcon) { imageClassName = "tippy-top-icon"; imageStyle = { backgroundColor: …, backgroundImage: … }; } else { imageClassName = `screenshot${link.screenshot ? " active" : ""}`; // can get rid of the const screenshotClassName imageStyle = {backgroundImage: …} }

that turned out nice like that 👍

Mardak · 2017-08-01T03:02:18Z

system-addon/lib/TippyTopProvider.jsm

+    // Load the Tippy Top sites from the json manifest.
+    fetch(TIPPYTOP_JSON_PATH)
+      .then(response => response.json())
+      .then(sites => {


These should be awaits given that we're inside an async function

try { for (const site of await (await fetch(PATH)).json()) { … } catch(error) …

Do I need to stub it out differently in tests to do this? I can't get them to work now for some reason.

oops nvm. there waz a typo

Mardak · 2017-08-01T03:09:04Z

system-addon/lib/TopSitesFeed.jsm

    for (let link of links) {
      if (!link) { continue; }
+
+      // Check for tippy top icon.
+      link = this._tippyTopProvider.processSite(link);


So, this seems like it could fail or give wrong results if TippyTopProvider hasn't finished adding to _sitesByDomain. I would guess the correct fix is to make processSite async. But we can skip this for now.

Yeah, figured it should just should just move on if it isn't fully init'd instead of waiting. I guess we would want to dispatch an action when init is done to refresh again. Maybe there should be a tippy top feed like you suggested 🤔

Well, Provider doesn't have the usual dispatch, but it async init returns a promise that TopSitesFeed could use to have refresh wait until Provider is inited . E.g.,

TopSitesFeed { constructor() { const provider = new TippyTopProvider(); this._initedProvider = provider.init(); // init needs to resolve the provider } async refresh() { … (await this._initedProvider).processSite(link);

This can be separately handled as a followup as I expect the tests would need to change a bit.

Also, it wouldn't allow for the "just move on" if that's desired.

cool, I'll look into that after doing the tippy top release for default sites

Mardak · 2017-08-01T03:12:31Z

system-addon/lib/TopSitesFeed.jsm

@@ -22,8 +23,11 @@ const DEFAULT_TOP_SITES = [];
 this.TopSitesFeed = class TopSitesFeed {
  constructor() {
    this.lastUpdated = 0;
+    this._tippyTopProvider = new TippyTopProvider();


@k88hudson any thoughts on having this TippyTopProvider vs a TippyTopFeed? If we do want a feed, we should just refactor it later. I could see the feed dispatching a mapping of domains to urls, but then again, would we want all that data going to stores and the content process?

Mardak · 2017-08-01T03:17:28Z

yamscripts.yml

+
+  copyTippyTopManifest: cpx "node_modules/tippy-top-sites/top_sites.json" system-addon/data/content/tippytop
+
+  copyTippyTopImages: cpx "node_modules/tippy-top-sites/images/**/*" system-addon/data/content/tippytop/images


nit: this seems better organized as

tippymc: pre: rimraf … manifest: cpx … images: cpx …

Mardak · 2017-08-01T03:17:48Z

yamscripts.yml

 # buildmc: Export the bootstraped add-on to mozilla central
  buildmc:
-    pre: rimraf ../mozilla-central/browser/extensions/activity-stream
+    pre: =>cleanmc && =>cleanTippyTop && =>copyTippyTopManifest && =>copyTippyTopImages


then this would just be => cleanmc && => tippymc

Mardak · 2017-08-01T03:43:25Z

system-addon/lib/TippyTopProvider.jsm

+          }
+          if ("urls" in site) {
+            for (let url of site.urls) {
+              this._sitesByDomain[getDomain(url)] = site;


nit: This seems a bit awkward with the repetition. Maybe

for (const url of site.url ? [site.url] : site.urls || []) { this._sitesByDomain.set(getDomain(url), site);

Or maybe that's just too clever :p It would want a comment ;) Also not quite exactly the same logic as it doesn't allow for both url and urls to exist..

I kind of likey 😉

Mardak · 2017-08-01T05:44:58Z

system-addon/lib/TopSitesFeed.jsm

  }
  init() {
+    this._tippyTopProvider.init();


Sorry for the conflict with removing TopSitesFeed.init Maybe just call this just after constructing it from TopSitesFeed.constructor ?

coveralls · 2017-08-01T14:15:10Z

Coverage remained the same at 86.407% when pulling 5aae927 on rlr:gh2944/tippy-tops into 729f4c6 on mozilla:master.

coveralls · 2017-08-01T14:24:09Z

Coverage remained the same at 86.407% when pulling 172a055 on rlr:gh2944/tippy-tops into 729f4c6 on mozilla:master.

as-pine-proxy · 2017-08-01T17:59:37Z

Treeherder: 7e3ee5c3f1d47e9f2e42812c0d65f55f05dd26b2

rlr added the PR / Needs review label Aug 1, 2017

rlr requested a review from Mardak August 1, 2017 02:44

rlr assigned Mardak and jaredlockhart Aug 1, 2017

Mardak approved these changes Aug 1, 2017

View reviewed changes

Mardak assigned rlr and unassigned jaredlockhart and Mardak Aug 1, 2017

Mardak added PR / R+ with nits and removed PR / Needs review labels Aug 1, 2017

Mardak mentioned this pull request Aug 1, 2017

Include tippy-top-sites of only the default sites #3051

Closed

Mardak reviewed Aug 1, 2017

View reviewed changes

Mardak mentioned this pull request Aug 1, 2017

Perma-orange: browser_all_files_referenced.js | there should be no unreferenced files - Got 193, expected 0 #3053

Closed

rlr force-pushed the gh2944/tippy-tops branch 2 times, most recently from f0de704 to 5aae927 Compare August 1, 2017 14:10

feat (systemaddon): mozilla#2944 add tippy top icons

172a055

rlr force-pushed the gh2944/tippy-tops branch from 5aae927 to 172a055 Compare August 1, 2017 14:18

rlr merged commit 1b4406c into mozilla:master Aug 1, 2017

Mardak mentioned this pull request Aug 1, 2017

Wait for TippyTopProvider to finish processing manifest before processing sites #3058

Closed

as-pine-proxy added the pushed-to-pine label Aug 1, 2017

rlr deleted the gh2944/tippy-tops branch August 1, 2017 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat (systemaddon): #2944 add tippy top icons #3049

feat (systemaddon): #2944 add tippy top icons #3049

rlr commented Aug 1, 2017 •

edited by Mardak

Loading

coveralls commented Aug 1, 2017

rlr commented Aug 1, 2017

Mardak left a comment

Mardak Aug 1, 2017 •

edited

Loading

Mardak Aug 1, 2017

Mardak Aug 1, 2017

Mardak Aug 1, 2017

Mardak Aug 1, 2017 •

edited

Loading

rlr Aug 1, 2017

Mardak Aug 1, 2017

rlr Aug 1, 2017

rlr Aug 1, 2017

Mardak Aug 1, 2017

rlr Aug 1, 2017

Mardak Aug 1, 2017 •

edited

Loading

rlr Aug 1, 2017

Mardak Aug 1, 2017

Mardak Aug 1, 2017

Mardak Aug 1, 2017

Mardak Aug 1, 2017

rlr Aug 1, 2017

Mardak Aug 1, 2017

coveralls commented Aug 1, 2017

coveralls commented Aug 1, 2017

as-pine-proxy commented Aug 1, 2017


		copyTippyTopManifest: cpx "node_modules/tippy-top-sites/top_sites.json" system-addon/data/content/tippytop

		copyTippyTopImages: cpx "node_modules/tippy-top-sites/images/*/" system-addon/data/content/tippytop/images

feat (systemaddon): #2944 add tippy top icons #3049

feat (systemaddon): #2944 add tippy top icons #3049

Conversation

rlr commented Aug 1, 2017 • edited by Mardak Loading

coveralls commented Aug 1, 2017

rlr commented Aug 1, 2017

Mardak left a comment

Choose a reason for hiding this comment

Mardak Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mardak Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mardak Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 1, 2017

coveralls commented Aug 1, 2017

as-pine-proxy commented Aug 1, 2017

rlr commented Aug 1, 2017 •

edited by Mardak

Loading

Mardak Aug 1, 2017 •

edited

Loading

Mardak Aug 1, 2017 •

edited

Loading

Mardak Aug 1, 2017 •

edited

Loading