feat (addon) #444 enhance sites with tippy top data #616
Conversation
| if (this.options.sites) { | ||
| sites = this.options.sites; | ||
| } else { | ||
| sites = JSON.parse(data.load("content/favicons/top_sites.json")); |
There was a problem hiding this comment.
Not sure if you want to touch the JSON file directly vs the require("tippy-top-sites") exported version via index.js (since we inject a couple values on the fly). /cc @k88hudson
There was a problem hiding this comment.
yeah i agree, i think it would be better to just require the file, since we might need stuff that gets added in there
There was a problem hiding this comment.
How do I do that? Do I need to copy over that index.js into the addon data directory? Can I use it from there with require? Since it's in node_modules, wasn't sure how.
There was a problem hiding this comment.
I think it's just a simple require("tippy-top-sites"), per ./content-test/faker.js:3.
There was a problem hiding this comment.
The issue with using require is that this is addon js. The tippy-top-sites module won't be available in the addon at runtime.
One way to get around this is to duplicate the module in a directory that will be packaged with the addon.
There was a problem hiding this comment.
Ah yes. This is a bit more verbose, but we can wire it up during the npm run bundle step:
const writeFile = require("fs").writeFileSync;
const join = require("path").join;
const tippy = require("tippy-top-sites");
const OUT_FILE = join("data", "content", "favicons", "top_sites.json");
writeFile(OUT_FILE, JSON.stringify(tippy));
| * Create a key using the domain (minus the www.). | ||
| */ | ||
| _getKey(url) { | ||
| let domain = URL(url).host; |
There was a problem hiding this comment.
I think we'll probably want to expand this to include all subdomains of a host as well, although that will result in some conflicts (e.g. aws.amazon.com and amazon.com are separate tippy-top entries)
There was a problem hiding this comment.
Yeah, I had talked about this with @oyiptong and we decided it was safer to match the full domain. I don't think there are that many cases in the top sites where we want to include all it's subdomains? I could be wrong.
|
YAY! This is awesome. A couple of small notes in the code. For the issue with matching subdomains, I think it's fine to do that as a follow-up. |
|
OK, I think I fixed the |
|
Ok, looks good to me! |
r? @oyiptong and/or @k88hudson
Fixes #444
This change is