fix (systemaddon): #3111 use screenshots instead of icons for default sites #3124
Conversation
|
Screenshot:
|
|
Sounds like "pantsuit nation" will be removed from the facebook one |
| @@ -46,10 +46,7 @@ class TopSite extends React.Component { | |||
| let imageStyle; | |||
| if (tippyTopIcon) { | |||
| imageClassName = "tippy-top-icon"; | |||
| imageStyle = { | |||
| backgroundColor: link.backgroundColor, | |||
| backgroundImage: `url(${tippyTopIcon})` | |||
There was a problem hiding this comment.
Looks like tests want backgroundColor. Could just put empty string (or leave it as undefined) and it should work?
There was a problem hiding this comment.
doh. yeah will just add a guard
There was a problem hiding this comment.
ah, leaving as undefined works fine. react ignores it and doesn't render it out 💃
|
haha, @bryanbell says he has a to fix a screenshot. guessing it's that |
rlr
force-pushed
the
default-sites-screenshots
branch
from
80545cb
to
28310c1
Compare
|
ok, I'll add a [WIP] to the title and a blocked tag we'll wait for the rest. |
rlr
changed the title
rlr
force-pushed
the
default-sites-screenshots
branch
from
28310c1
to
e43aa55
Compare
| [ | ||
| { | ||
| "title": "amazon", | ||
| "urls": ["https://www.amazon.com/", "https://www.amazon.ca/", "https://www.amazon.de/", "https://www.amazon.co.uk/", "https://www.amazon.fr/"], |
There was a problem hiding this comment.
This won't work because the screenshots have english text in them.
| { | ||
| "title": "facebook", | ||
| "url": "https://www.facebook.com/", | ||
| "image_url": "facebook-com@2x.png" |
There was a problem hiding this comment.
Not sure what we'll do for other languages that use facebook.com, but the screenshot is english.
There was a problem hiding this comment.
oy, this is becoming more work than probably expected. Is the country in the redux state somewhere?
There was a problem hiding this comment.
@k88hudson Not sure how long term this change would be, but one quick hack to get geo/country code into content would be to set it as a getValue pref:
["geo", {
getValue: ({geo}) => geo
}]And then content can access it via Prefs from the store.
Although, don't you actually want the geo from TopSitesFeed or TippyTopProvider? Those could just directly access Services.prefs.getStringPref("browser.search.region"). But if we do want geo everywhere, it should probably go in store.App alongside locale ?
There was a problem hiding this comment.
Good point, I would only need it from TippyTopProvider for this. 👍
There was a problem hiding this comment.
It seems reasonable to access it directly here, but I wonder if we need to solve this issue more broadly for things down the line? adding it to store.Appmakes sense, although ordering would still be an issue I guess
There was a problem hiding this comment.
The current plan is to allow english screenshots for those who share the same domain, e.g., facebook.com. There will be multiple screenshots for amazon as there's amazon.com and amazon.de, etc. This means we don't need to get the country code and just check the domain string.
rlr
force-pushed
the
default-sites-screenshots
branch
from
e43aa55
to
db6c67d
Compare
rlr
changed the title
|
@k88hudson this is ready to go now. I manually verified locally that all the regions get a resource:// screenshot. final r? |
|
Ok this looks great! Let's just wait until #3186 to land this 👍 |
k88hudson
dismissed
Mardak’s stale review
Picking up this review since Mardak is on PTO
This only fully covers en-US default top sites. @bryanbell says to ship these and the others will come shortly.
r? @Mardak
Fixes #3111