mozilla / tippy-top-sites-deprecated

[deprecated][unmaintained]
6 stars 7 forks source link

Add byUrl, all property instead of a single array #32

Closed k88hudson closed 8 years ago

k88hudson commented 8 years ago

This adds a byUrl property so that items can be easily accessed by url

k88hudson commented 8 years ago

@nchapman / @pdehaan R?

k88hudson commented 8 years ago

Note that this is a breaking change, so should be a major release

pdehaan commented 8 years ago

Looks good, do we want to normalize keys for byAll though?

So, instead of a key like "http://www.about.com/", we'd just pretty it up to be "about.com" (and similarly, instead of "https://aws.amazon.com/" we use "aws.amazon.com"). That may be more flexible for http vs https and optional "www." subdomains.

  "byUrl": {
    "http://www.about.com/": {
      "title": "About",
      "url": "http://www.about.com/",
      "image_url": "about-com.png",
      "background_color": "#FFF",
      "background_color_rgb": [
        255,
        255,
        255
      ]
    },
    "http://www.adobe.com/": {
      "title": "Adobe",
      "url": "http://www.adobe.com/",
      "image_url": "adobe-com.png",
      "background_color": "#e22919",
      "background_color_rgb": [
        226,
        41,
        25
      ]
    },
    ...
pdehaan commented 8 years ago

Ah, and while we ponder my vast stupidity, it looks like the moar-sites branch has a nifty domain property which we could use for the pretty key:

  {
    "title": "Amazon",
    "url": "http://www.amazon.com/",
    "image_url": "amazon-com.png",
    "background_color": "#FFF",
    "domain": "amazon.com"
  },
  {
    "title": "Amazon Web Services",
    "url": "https://aws.amazon.com/",
    "image_url": "amazonaws-com.png",
    "background_color": "#FFF",
    "domain": "aws.amazon.com"
  },
k88hudson commented 8 years ago

@pdehaan yeah that's probably a better idea

pdehaan commented 8 years ago

Or we can do it in a future PR. The moar-sites branch hasn't been merged yet. This looks good to me, and you're free to merge whenever (if we need this right meow for the internal beta).

pdehaan commented 8 years ago

Not sure if we should put domain in the JSON file directly, or if we should inject it via index.js' module.exports, similar to how we do with background_color_rgb:

{
  "title": "Amazon Web Services",
  "url": "https://aws.amazon.com/",
  "image_url": "amazonaws-com.png",
  "background_color": "#FFF",
  "domain": "aws.amazon.com"
}

versus something like this :poop::

const sites = require("./top_sites.json").map(site => Object.assign({}, site, {
  background_color_rgb: hexToRgb(site.background_color),
  domain: prettyUrl(site.url)
}));

Pros: Less likely to make mistakes/typos/copy-pasta. Cons: Less customizable.


UPDATE: Looks like we currently have 3 sites in moar-sites branch which have a mismatch between the url and pretty domain:

  1. discovercard.com vs discover.com — note that we also have a result for discover.com here.
  2. mlb.com vs mlb.mlb.com
  3. nordstrom.com vs shop.nordstrom.com
const data = require("./top_sites.json");
const url = require("url");

const prettyUrl = (site) => url.parse(site.url).host.replace(/^www\./, "");

data.forEach((site) => {
  if (site.domain !== prettyUrl(site)) {
    console.error("BOOM!", site.domain, prettyUrl(site));
  }
});