mozilla / tippy-top-sites-deprecated

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

Allow multiple URL patterns for Tippy Top entries to cover cases like gmail.com + mail.google.com #41

Closed pdehaan closed 7 years ago

pdehaan commented 7 years ago

Ref: https://github.com/mozilla/activity-stream/issues/1311

Allow multiple URL patterns for Tippy Top entries to cover cases like gmail.com + mail.google.com

rlr commented 7 years ago

There are different approaches to this. The two main ones I can think of are:

1) The naive no code change required approach is just to add a new entry for each URL pattern. It just would be a little repetitive, but I don't know how many of these cases we are thinking about. It might not be too bad? So, we would have:

{
    "title": "Gmail",
    "url": "https://mail.google.com/",
    "image_url": "google-gmail.png",
    "background_color": "#FFF",
    "domain": "google.com"
},
{
    "title": "Gmail",
    "url": "https://gmail.com/",
    "image_url": "google-gmail.png",
    "background_color": "#FFF",
    "domain": "google.com"
},

2) Or we could remove/deprecate the url property and add a urls property that's an array of URLs. This would mean we need to change Activity Stream to handle this new property properly and coordinate landing that with a new version of the tippy tops json.

{
    "title": "Gmail",
    "urls": ["https://mail.google.com/", "https://gmail.com/"],
    "image_url": "google-gmail.png",
    "background_color": "#FFF",
    "domain": "google.com"
},

Any thoughts, preferences, different ideas, etc? @pdehaan @k88hudson @dmose

pdehaan commented 7 years ago

I like option 2, although agree that it does have a higher coordination factor. Semi-related, but looking at the examples above not sure if we can remove the protocol too. I always get confused by sites like reddit where we'd potentially have a explosive combination of:

http://reddit.com http://www.reddit.com https://reddit.com https://www.reddit.com

But I don't think we can embed some clever regexs in the JSON to support stuff like /https?://(www\.)?reddit\.com/i

rlr commented 7 years ago

Good point. On the Activity Stream side, we only look at the host and removing the protocol shouldn't break how we are parsing it. I agree though, we should remove that. Regex is another option but I'll only go there if it looks really necessary.

pdehaan commented 7 years ago

I don't think JSON natively supports RegExp, so we'd have to probably store the regular expression as a string in the JSON and do some magic on the Activity Stream side when parsing the tippy-top-sites JSON blob.

rlr commented 7 years ago

I discussed this with @k88hudson and we decided to add the url to site matching logic to this repo and keep the data structure changes minimal. After that we can go nuts with data structure changes without affecting Activity Stream or any other app that wants to use this data.

pdehaan commented 7 years ago

Fixed via #48. Closing.