Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Bug 1467485 - Missing screenshots for TopSiteLink components #4198

Merged
merged 1 commit into from Jun 8, 2018

Conversation

imjching
Copy link
Contributor

@imjching imjching commented Jun 7, 2018

This fixes the bug in which screenshots for TopSiteLink components are missing.

Before

topsites_before

After

topsites_after

This uses the same logic as the one in #4150. I have extracted out the screenshot logic from the Card component into ScreenshotUtils so that it can be used in the TopSiteLink component as well. The naming in ScreenshotUtils might be confusing at first (RemoteImage, LocalImage). I have added a block of comment in ScreenshotUtils to address that:

/**
 * List of helper functions for screenshot-based images.
 *
 * There are two kinds of images:
 * 1. Remote Image: This is the image from the main process and it refers to
 *    the image in the React props. This can either be an object with the `data`
 *    and `path` properties, if it is a blob, or a string, if it is a normal image.
 * 2. Local Image: This is the image object in the content process and it refers
 *    to the image *object* in the React component's state. All local image
 *    objects have the `url` property, and an additional property `path`, if they
 *    are blobs.
 */

I'm not quite sure if there's a better naming for those, and I am open to suggestions. 😀

@k88hudson k88hudson self-requested a review June 7, 2018 20:24
isBlob: _isBlob,

// This should always be called with a remote image and not a local image.
createLocalImageObject: remoteImage => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason these aren't just regular functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these were all regular methods on ScreenshotUtils, then there wouldn't need to be a separate _isBlob and a plain isBlob method called from these other methods as this.isBlob(…) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Will update accordingly. 👍

Copy link
Contributor

@k88hudson k88hudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, looks good to me. Thanks for fixing this, sorry I missed this in the original review!

Signed-off-by: Jay Lim <6175557+imjching@users.noreply.github.com>
@imjching
Copy link
Contributor Author

imjching commented Jun 7, 2018

Updated the PR to address the comments.

@Mardak Mardak merged commit b92c8c6 into mozilla:master Jun 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants