fix(highlights): Add image caching and image placeholders. #3427
Conversation
|
Design says to have a blended border that shares the gradient. I mocked it quickly with a box-shadow inset as borders go outside of the image:
Design says ideally no extra shadow creeping in from left/right edges but if that can't be fixed, then it's still better than the solid grey-30 border along the bottom. |
|
Oh, here we go with no side shadow: .card-outer .card-preview-image-outer::after {
border-bottom: 1px solid rgba(0, 0, 0, 0.05);
bottom: 0;
content: " ";
position: absolute;
width: 100%;
}With the
|
There was a problem hiding this comment.
It looks like there were some decisions made to keep backwards compatibility with other Sections. What compatibility do we actually need to keep vs having the code the way we want? E.g., instead of just relying on a hasImage, we need to additionally check image (which could be renamed to imageUrl) for Sections that only provided an image.
Also, what's the cache supposed to help with?
system-addon/lib/HighlightsFeed.jsm
Outdated
| // We want the page, so update various fields for UI | ||
| Object.assign(page, { | ||
| image, | ||
| imageLoading: true, // We always have an image - fall back to a screenshot |
There was a problem hiding this comment.
I would assume this imageLoading is for other sections to have just text? Let's just rename this to hasImage as it would seem to imply it should be false if it's already loaded.
| {link.image && <div className="card-preview-image" style={{backgroundImage: `url(${link.image})`}} />} | ||
| <div className={`card-details${link.image ? "" : " no-image"}`}> | ||
| {hasImage && <div className="card-preview-image-outer"> | ||
| <div className={`card-preview-image${link.image ? " active" : ""}`} style={imageStyle} /> |
There was a problem hiding this comment.
The use of "active" here is confusing as at least with css-related stuff, it's when the user is interacting with it. Probably name this "loaded".
| @@ -56,14 +56,18 @@ class Card extends React.Component { | |||
| const isContextMenuOpen = this.state.showContextMenu && this.state.activeCard === index; | |||
| // Display "now" as "trending" until we have new strings #3402 | |||
| const {icon, intlID} = cardContextTypes[link.type === "now" ? "trending" : link.type] || {}; | |||
| const hasImage = link.image || link.imageLoading; | |||
There was a problem hiding this comment.
With imageLoading renamed to hasImage, I think we should just rely on that value instead of additionally checking image, so const {hasImage} = link;
There was a problem hiding this comment.
Oh. Although I see you probably did this for backwards compatibility for other things passing in just image…
There was a problem hiding this comment.
Yeah because pocket for example doesn't set hasImage
system-addon/lib/HighlightsFeed.jsm
Outdated
| @@ -81,6 +90,20 @@ this.HighlightsFeed = class HighlightsFeed { | |||
|
|
|||
| SectionsManager.updateSection(SECTION_ID, {rows: this.highlights}, this.highlightsLastUpdated === 0 || broadcast); | |||
| this.highlightsLastUpdated = Date.now(); | |||
| this.imageCache.clear(); | |||
There was a problem hiding this comment.
What's the purpose of the cache if we clear it on every fetch? It seems like it would only be beneficial if multiple highlights ended up using the same image?
There was a problem hiding this comment.
The image cache cleared at the end of every fetchHighlights once it's been used (in that method). The async fetchImage calls then fill up the cache again for the next fetchHighlights call. I think, though I could be wrong, that none of the fetchImage calls are executed before fetchHighlights has finished executing, so they fill up the cache after it has been cleared. Without the image cache I found that subsequent refreshes caused flickering of images for existing cards as the src was unset then reset again asynchronously.
There was a problem hiding this comment.
Hrm.. That makes sense. Although it's also quite dangerous how it is right now. If something were to delay this clear() until after some images were fetched and set, then it's just throwing away that work.
The cache seems to be acting more as a prefetcher, but it could be a better actual cache if it only removed items that weren't used if the concern is holding on to fetched images too long. But this can be done in a followup bug.
At minimum better comment on why it's clearing here and how the cache will still be useful.
system-addon/lib/SectionsManager.jsm
Outdated
| updateSectionCard(id, url, options, shouldBroadcast) { | ||
| if (this.sections.has(id)) { | ||
| const card = this.sections.get(id).rows.find(elem => elem.url === url); | ||
| if (card) { Object.assign(card, options); } |
There was a problem hiding this comment.
nit: put the if code block body on its own line
|
Oh and we'll want to pass in backgroundColor of grey-10 #f9f9fa |
Closes #2322. Closes #3406.