Fix Bug 1402654 - Allow custom image / thumbnail for an edited top site #3891

piatra · 2017-12-04T15:41:28Z

Closes https://bugzilla.mozilla.org/show_bug.cgi?id=1402654

How it works

when the user inputs a url and makes a preview request we dispatch SCREENSHOT_REQUEST that gets handled in the TopSitesFeed getScreenshotPreview. this makes a Screemshot.getScreenshotForURL request and dispatches a SCREENSHOT_UPDATE with the response (or SCREENSHOT_FAILED).
the TopSites reducer now handles SCREENSHOT_UPDATE (for different kinds of paylods)
- it can set screenshotPreview or customScreenshot in addition to screenshot to a Topsite.
when you update a topsite that includes a custom url, we have to clear the previous cached value from the LinksCache object
when we query for topsites we only fetch the custom screenshot if it is set (so no rich icons or regular screenshot or favicon)
the url value is stored as customScreenshotURL in the pinned pref
because you have to click “save” to set a custom screenshot, all topsites with a custom screenshot are pinned sites
The preview form uses the same component we use to display a Topsite item. There are 2 extra cases to handle:
- request failure -> letter fallback
- custom screenshot screenshotPreview || customScreenshot
for the Form inputs refactoring, the new component TopSiteEditInputs includes:
- the url, title and custom screenshot inputs
- The component handles the logic to display the error labels and to focus the inputs. Also the toggle to show hide the custom screenshot input is here. All the validation and the input change handler are still in the TopsiteForm.

UI/UX

There is no one source of information but I will try to list everything.
Latest design https://mozilla.invisionapp.com/share/7DEP1VMYX#/screens/266461200_Top_Site_-_Edit_Image_
In addition to that:

Adding the tab loading animation when fetching a url
Adding an X button at the end to clear the input
- spec here https://design.firefox.com/photon/components/input-fields.html#behaviors
Change the input focus behavior to match the same spec from design.firefox.com (blue border&shadow)
The behavior we want for the screenshot input is:
- hidden by default (unless user value is set)
- click the link to reveal it (no option/action to hide back)
Add input button to default site context menu (without having to go through the "Edit Mode")

Desired but not implemented:

Adding a fade effect for text overflow (similar how location bar is doing it)
- https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/base/content/browser.css#561
- but there is no spec for how the "x" and the fade would work together
- the location bar achieves the effect without actually using an input element
- not clear what other input fields should get the fade effect

No required:

the new design of the buttons
- in the invision spec the cancel/save buttons look different
- that design is not final yet so we are still using our current buttons

Let me know if I skipped something.

Mardak · 2017-12-05T04:38:18Z

@aaronrbenson What's the expected behavior for various interactions and does this current PR look correct?

This used to have the page's screenshot, but when the image failed, it switched to just the letter fallback. The change to letter could be useful as an extra indication that it attempted. Also, is the failure message text and sizing correct -- I don't see that message in the invision doc:

The preview link should not be interact-able and should have the image cropped differently? (Currently you can click it, and it navigates.)

This shows up when deleting the content of the input box:

I guess we shouldn't show the old screenshot when the url is cleared? Also, the sizing of the text and the string for the custom image should have ellipsis…? Also, what should cause the custom image input to disappear? Right now moving focus away from the image input box when there's no value makes it disappear.

Is this the desired layout with the URL red error message?

We probably want a non-text-selection cursor for hovering… and the color should be?

The size of the confirmation button changes widths when it goes from Preview to Save, but probably not too much we can do there while not causing oddness for l10n?

Mardak

The first thing I tried to test was editing a default top site's image, and it kept showing the loading animation forever. I'm guessing it's because the default top site isn't handled in setCustomTopsiteScreenshot.

But setCustomTopsiteScreenshot seems to have some odd behaviors of broadcasting and updating main redux state for a transient image preview request.

@k88hudson do you have suggestions on how to better handle all of this state?

Also, looks like things don't quite match the invision doc in terms of layout, strings, sizing, behavior, etc.; and some things don't seem quite defined by design either, so followup there too.

Mardak · 2017-12-05T05:36:12Z

locales/en-US/strings.properties

@@ -146,6 +146,13 @@ topsites_form_add_button=Add
 topsites_form_save_button=Save
 topsites_form_cancel_button=Cancel
 topsites_form_url_validation=Valid URL required
+topsites_form_label_title=Title
+topsites_form_label_url=URL


nit: let's put these next to their related placeholders, so group together topsites_form_title_* and _url_*:

…title_label=… …title_placeholder=…

Mardak · 2017-12-05T05:36:12Z

locales/en-US/strings.properties

@@ -146,6 +146,13 @@ topsites_form_add_button=Add
 topsites_form_save_button=Save
 topsites_form_cancel_button=Cancel
 topsites_form_url_validation=Valid URL required
+topsites_form_label_title=Title
+topsites_form_label_url=URL
+topsites_form_label_screenshot_url=Custom Image


Probably use topsites_form_image_label or at least not use "screenshot" as that could be confusing for localizers looking at the name of the string as it doesn't seem to relate to the string

Mardak · 2017-12-05T05:36:12Z

system-addon/common/Reducers.jsm

@@ -95,18 +95,25 @@ function TopSites(prevState = INITIAL_STATE.TopSites, action) {
      }
      return Object.assign({}, prevState, {initialized: true, rows: action.data});
    case at.TOP_SITES_EDIT:
-      return Object.assign({}, prevState, {editForm: {visible: true, site: action.data}});
+      return Object.assign({}, prevState, {editForm: {visible: true, site: action.data}}, {screenshotRequestFailed: false});


Any reason to add a new object here instead of just adding the property to the existing one that has editForm?

Mardak · 2017-12-05T05:36:12Z

system-addon/content-src/components/TopSites/TopSiteForm.jsx

+      // cached results from previous requests.
+      screenshotPreview: null,
+      showCustomScreenshotForm: false,
+      screenshotRequestFailed: false


Something about this doesn't seem right with the Reducers.jsm change also tracking some of the same state? Should more of this be in the redux state instead of this local state? Or some other way to track all of this? I suppose one aspect is that only one form for a given site would be open at any time? @k88hudson

Current implementation adds:
In the TopSiteForm:

customScreenshotUrl - this is more or less required for the input field to work + to compare with the prop and see if the input value changed

pendingScreenshotUpdate - to shows the loading indicator

screenshotPreview - this is a side effect of the BroadcastToContent (maybe I'm missing something): the user can exit the preview/edit flow by just refreshing the page so we don't have a chance to clear props.Topsite.screenshotPreview set in the Reducer. If you click edit again you will be presented with the previous preview image but the url associated is lost. Having this locally means it automatically gets cleared when the component dismounts.

topsiteUrlValidationError & screenshotUrlValidationError - This way I can notify the TopSiteEditInputs component of validation errors (to show error labels & focus the elements) while at the same time keep the validation methods in the TopSiteForm component where they are needed more (add/save/preview functionality).

In TopSiteEditInputs

showCustomScreenshotForm - to force the input to show

Mardak · 2017-12-05T05:36:12Z

system-addon/lib/TopSitesFeed.jsm

@@ -293,6 +335,9 @@ this.TopSitesFeed = class TopSitesFeed {
      case at.TOP_SITES_UNPIN:
        this.unpin(action);
        break;
+      case at.SCREENSHOT_REQUEST:
+        this.setCustomTopsiteScreenshot(action.data);


This is called on a preview, so calling a set would seem to be more permanent than a preview. So either the method is not actually permanent or it shouldn't be as permanent?

s/set/get since the function now calls getScreenshotForURL.

Mardak · 2017-12-05T05:36:12Z

system-addon/lib/TopSitesFeed.jsm

+      Screenshots.maybeCacheScreenshot(link, customScreenshotURL, "screenshotPreview",
+        // When done, broadcast the result of the request.
+        screenshotPreview => {
+          this.store.dispatch(ac.BroadcastToContent({


We probably don't need to broadcast to all pages when one form on a page is requesting the image? This would also change the main store's state which doesn't seem right for a transient image preview?

@Mardak I agree with the comment but I'm not sure how to address this. Any advice on how to pass the message?

I believe @sarracini was looking at adding something like SendOnlyToContent in https://bugzilla.mozilla.org/show_bug.cgi?id=1415491 but before then, I suppose the closest would be SendToContent, but that would still update main store.

@k88hudson is there a preferred way to send transient data to a single content page without updating main store?

the Broadcast has remained the same atm.

Mardak · 2017-12-05T05:36:12Z

system-addon/lib/TopSitesFeed.jsm

@@ -165,19 +169,22 @@ this.TopSitesFeed = class TopSitesFeed {
   * Get an image for the link preferring tippy top, rich favicon, screenshots.
   */
  async _fetchIcon(link) {
+    const hasCustomScreenshot = !!link.customScreenshotURL;


We should probably short circuit first if it has a custom screenshot and do some custom behavior. Or maybe fetchIcon just shouldn't be called if we aren't using an icon anyway?

Now this never gets called if customScreenshotURL is set, instead if the customScreenshot is missing it will trigger a maybeCacheScreenshot.

Mardak · 2017-12-05T05:36:12Z

system-addon/lib/Screenshots.jsm

+  */
+  async replaceScreenshot(newScreenshotURL, url) {
+    // .copy() will overwrite any existing thumbnail associated with `url`
+    await PageThumbsStorage.copy(newScreenshotURL, url);


This doesn't seem right. There are other consumers of PageThumbs, so having it show some custom image that is not the page's thumbnail will probably lead to bugs. Highlights doesn't replace a url's thumbnail with the preview_image_url by requesting the thumbnail for that image instead of the page. Maybe something like that could be used for these custom preview images for top sites?

Changed this. Now a preview request will trigger a getScreenshotForURL and we attach that in the Reducer to the Topsite as screenshotPreview.
If the user saves the form (it pins the site) we save the URL in the pinned pref as customScreenshotURL and upon the feed refresh we trigger maybeCacheScreenshot which adds customScreenshot to the LinksCache object that represents that Topsite. The customScreenshot is also migrated between links so we need to clear the value if the user updated the customScreenshotURL for the website.

Mardak · 2017-12-05T05:36:12Z

system-addon/content-src/components/TopSites/TopSiteForm.jsx

    };
    this.onLabelChange = this.onLabelChange.bind(this);
    this.onUrlChange = this.onUrlChange.bind(this);
    this.onCancelButtonClick = this.onCancelButtonClick.bind(this);
+    this.onCustomScreenshotUrlChange = this.onCustomScreenshotUrlChange.bind(this);
+    this.onEnableScreenshotUrlForm = this.onEnableScreenshotUrlForm.bind(this);
+    this.onHideCustomScreenshotForm = this.onHideCustomScreenshotForm.bind(this);


With the 3 of these label/inputs, it would seem natural to move to a component

The 3 here I was referring to the 3 label/inputs: title label/input, url label/input, image label/input

Created TopSiteEditInputs.jsx

includes the url, title and custom url inputs

handles the logic to display the error labels and to focus the inputs

toggle to show hide the custom screenshot input

All the validation and the input change handler are still in the TopsiteForm.

Mardak · 2017-12-05T05:36:12Z

system-addon/content-src/components/TopSites/TopSite.jsx

-    imageClassName = `screenshot${link.screenshot ? " active" : ""}`;
-    imageStyle = {backgroundImage: link.screenshot ? `url(${link.screenshot})` : "none"};
+  // If the screenshot request failed use letter fallback.
+  if (!screenshotRequestFailed) {


Something about this seems unnecessarily complex,but I'll need to think about it a bit more. I suppose we should be able to avoid all the lines of change by having a empty

if (screenshotRequestFailed) { // Just use the letter fallback } else if (screenshotPreview…) { } else if (tippyTopIcon …) {

Changed as suggested.

aaronrbenson · 2017-12-05T14:47:33Z

Could I get a build to do some UX quality checking for this? There are a lot of little interactions and behaviors that @Mardak raises above and it would be helpful if I could test it out locally to provide notes.

pdehaan · 2017-12-05T17:28:54Z

locales/en-US/strings.properties

+topsites_form_label_screenshot_url=Custom Image
+topsites_form_screenshot_request_error=Image failed to load. Try a different URL.
+topsites_form_activate_custom_screenshot=Use a custom image
+topsites_form_custom_screenshot_placeholder=Paste a URL here


Why does one placeholder say "Type or paste" and the other is just "Paste"? Can't you type into the topsites URL?

topsites_form_url_placeholder=Type or paste a URL topsites_form_custom_screenshot_placeholder=Paste a URL here

I'll bring this up for discussion.

Mardak · 2017-12-07T06:30:18Z

@piatra could you comment on each review comment on what changed? It's hard to track down what's going on with so many commits with it already being a large change. Also, it would have been helpful to have a summary of what the approach is in the initial PR message.

Did you get UX feedback? And if so, what are the decisions?

piatra · 2017-12-07T09:26:48Z

@Mardak will do.
No UX feedback yet, will be doing that next week in person.

k88hudson · 2017-12-08T20:49:17Z

@Mardak @piatra I am still reading through, but I would think that any kind of preview-related state change should stay in the local redux context (using plain actions without SendToMain etc.). It's possible that maybe a content-only reducer/part of a reducer is needed?)

sarracini · 2017-12-14T20:26:30Z

system-addon/lib/Screenshots.jsm

@@ -12,6 +12,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "BackgroundPageThumbs",
  "resource://gre/modules/BackgroundPageThumbs.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PageThumbs",
  "resource://gre/modules/PageThumbs.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PageThumbsStorage",
+  "resource://gre/modules/PageThumbs.jsm");


where are you using PageThumbsStorage?

@sarracini the implementation changed a few times around and I forgot to remove it. Done. Thanks.

Mardak · 2017-12-21T07:01:06Z

Are there updated designs or a description of how things should be changed?

piatra · 2017-12-21T08:54:53Z

There is no one source of information but I will try to list everything.
Latest design https://mozilla.invisionapp.com/share/7DEP1VMYX#/screens/266461200_Top_Site_-_Edit_Image_
In addition to that:

Adding the tab loading animation when fetching a url
Adding an X button at the end to clear the input
- spec here https://design.firefox.com/photon/components/input-fields.html#behaviors
Change the input focus behavior to match the same spec from design.firefox.com (blue border&shadow)
The behavior we want for the screenshot input is:
- hidden by default (unless user value is set)
- click the link to reveal it (no option/action to hide back)
Add input button to default site context menu (without having to go through the "Edit Mode")

Desired but not implemented:

Adding a fade effect for text overflow (similar how location bar is doing it)
- https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/base/content/browser.css#561
- but there is no spec for how the "x" and the fade would work together
- the location bar achieves the effect without actually using an input element
- not clear what other input fields should get the fade effect

No required:

the new design of the buttons
- in the invision spec the cancel/save buttons look different
- that design is not final yet so we are still using our current buttons

Let me know if I skipped something.

piatra · 2018-01-08T08:52:49Z

I've rebased the patch against the latest drag and drop work. Waiting on review.

I still think there is a bug in the LinksCache causing an issue with this PR, I've added a test (the one failing) that tries to capture the behavior.

activity-stream/system-addon/lib/LinksCache.jsm

Lines 109 to 111 in 84fd287

    
           updateLink(property, value) { 
        
             newLink[property] = value; 
        
           }

I think the problem is that updateLink always references the first newLink object created (even after the property is migrated) so calling expire and then updateLink will not update the newest object.

@Mardak / @k88hudson can you help validate this? What I’ve done is tag every link object with an incrementing id and then track who is being updated in the updateLink call.
Should it be this[prop] instead of newLink[prop] what we use when updating ?

In this video https://ed.agadak.net/as/activity-stream.edit-preview.webm you do not see the bug because the sequence of events is:

set custom avatar
- (we are still working with the first link object created)
- inside the pin method we calll updateLink to set customScreenshot to some "falsy" value to trigger a fetch
- pinning happens that triggers an expire call
refresh is called/getLinksWithDefaults
- migration will copy over the "falsy" customScreenshot value
- calls to maybeCacheScreenshot will not set the customScreenshot value on the correct object, but it will broadcast the image and refresh the UI
  The custom image actually changes because the link object will always have an undefined custom screenshot (thus triggering a fetch every time). But if a different topsite is edited it won't work.

When you add a new Topsite maybeCacheScreenshot works because it sets the screenshot property on the newly created link object.

Here is another video actually showing it fail when we first pin a site (to trigger expire) and then try to edit https://drive.google.com/open?id=1KRyOoqb_Rg2UgFbktYZDhLlotFdKTOko

Mardak · 2018-01-09T00:55:40Z

I think the problem is that updateLink always references the first newLink object created (even after the property is migrated) so calling expire and then updateLink will not update the newest object.

Ah indeed. Looks like we want to keep the original __sharedCache object (to maintain sharing) while updating the function to refer to the new link.

piatra · 2018-01-09T10:35:57Z

Should this PR fix it or have it done separately?

Mardak

Still working through this PR as there's 39 commits and various changes that could have been separated. Ideally it should have been broken up into some commits that would have been individually easier to review, e.g.:

refactor to TopSiteEditInput
add link preview to edit form
add custom screenshot input / saving
add screenshot previewing
update telemetry

Mardak · 2018-02-16T16:48:51Z

This will need to be rebased on top of #3965.

https://design.firefox.com/photon/patterns/inactive.html

…ital image

* TopSites reducer clears screenshotPreview and requestFailed status when necessary * Added action for content process to update its own reducer when a request is canceled

piatra · 2018-03-21T20:00:07Z

I'm not sure if d7d40fa is what you meant in this comment I didn't understand the reference to TOP_SITES_EDIT

Ah good point. I think this is a neat way to avoid updating main. Where TOP_SITES_EDIT to trigger reducing these actions is only seen in content.

There might be a weird edge case here (I'm still thinking about it):

PREVIEW_RESPONSE sets props (previewResponse: "data", previewUrl: "url")
Deleting a character will send PREVIEW_REQUEST_CANCEL => (previewResponse: null, previewUrl: "url")
Add that character back => you match the loading condition & trigger the loading animation

So what I did was remove previewUrl in the PREVIEW_REQUEST_CANCEL action

Still waiting to hear on the decision about reverting from an error state

Mardak · 2018-03-21T20:18:12Z

What it was before should have been fine, i.e., AlsoToMain(PREVIEW_REQUEST). Just the one PREVIEW_REQUEST that both content and main handle. Content would have had a previous TOP_SITES_EDIT allowing Reducers to update editForm while main would skip changing state (but TopSitesFeed would request a screenshot).

This reverts commit d7d40fa.

Mardak

This should be good with some final nits! Thanks!

Mardak · 2018-03-22T05:21:44Z

system-addon/common/Actions.jsm

+function OnlyToThisContent(action) {
+  return _RouteMessage(action, {
+    from: CONTENT_MESSAGE_TYPE,
+    to: CONTENT_MESSAGE_TYPE


Did this OnlyToThisContent get added back? I thought it was removed

Mardak · 2018-03-22T05:21:44Z

system-addon/common/Reducers.jsm

+          editForm: {
+            index: prevState.editForm.index,
+            previewResponse: action.data.preview,
+            previewUrl: action.data.url


Shouldn't need to re-assign the values for index and previewUrl as they're the same as prevState's. Although we end up with nested Object.assigns.. not sure if that means we should be making the state flatter? @k88hudson ?

return Object.assign({}, prevState, {editForm: Object.assign({}, prevState.editForm, {previewResponse: action.data.preview})});

Had that at first but makes it more difficult to read. I also think it helps in this case to be explicit about what we want our props to be (for debugging/refactoring reasons).

Mardak · 2018-03-22T05:21:44Z

system-addon/common/Reducers.jsm

    case at.TOP_SITES_CANCEL_EDIT:
      return Object.assign({}, prevState, {editForm: null});
+    case at.PREVIEW_RESPONSE:
+      if (prevState.editForm &&
+        prevState.editForm.previewUrl === action.data.url) {


nit: The other actions short circuit to return prevState, so let's make this follow that pattern too.

if (!prevState.editForm || action.data.url !== prevState.editForm.previewUrl) { return prevState; }

Mardak · 2018-03-22T05:21:44Z

system-addon/content-src/components/TopSites/TopSite.jsx

+    if (defaultStyle) { // Render the Topsite without any image or icon
+      smallFaviconFallback = false;
+    } else if (link.customScreenshotURL) {
+      // styles and class names for top sites with custom screenshot


This comment doesn't really explain why we're using ".rich-icon" for custom screenshots

Mardak · 2018-03-22T05:21:44Z

system-addon/content-src/components/TopSites/TopSite.jsx

@@ -67,7 +67,16 @@ export class TopSiteLink extends React.PureComponent {
    let showSmallFavicon = false;
    let smallFaviconStyle;
    let smallFaviconFallback;
-    if (tippyTopIcon || faviconSize >= MIN_RICH_FAVICON_SIZE) {
+    if (defaultStyle) { // Render the Topsite without any image or icon


This comment style doesn't match the rest. Would be good to hint at "even if the link has imagery"

Mardak · 2018-03-22T05:21:44Z

system-addon/content-src/components/TopSites/TopSiteForm.jsx

+
+  _updateCustomScreenshotInput(value) {
+    this.setState({
+      customScreenshotUrl: value,


nit: if you name the parameter as customScreenshotUrl, then you can omit the : value

Mardak · 2018-03-22T05:21:44Z

system-addon/content-src/components/TopSites/TopSiteForm.jsx

+      customScreenshotUrl: value,
+      validationError: false
+    });
+    this.props.dispatch(ac.OnlyToThisContent({type: at.PREVIEW_REQUEST_CANCEL}));


Odd. This was removed before in piatra@18b0416

Mardak · 2018-03-22T05:21:44Z

system-addon/content-src/components/TopSites/TopSiteForm.jsx

+    const changed = customScreenshotUrl && this.cleanUrl(customScreenshotUrl) !== previous;
+    // Preview mode if changes were made to the custom screenshot URL and no preview was received yet
+    // or the request failed
+    const previewMode = (changed && this.props.previewResponse === null) || requestFailed;


This should be able to be just previewMode = changed && !this.props.previewResponse If the value wasn't changed, we shouldn't be looking for a preview request.

(The input change dispatching PREVIEW_REQUEST_CANCEL causes requestFailed to get cleared.)

piatra requested a review from Mardak Dec 4, 2017

piatra assigned Mardak Dec 4, 2017

piatra added the PR / Needs review label Dec 4, 2017

Mardak changed the title ~~Add custom image picker for topsites~~ Fix Bug 1402654 - Allow custom image / thumbnail for an edited top site Dec 4, 2017

Mardak suggested changes Dec 5, 2017

View changes

pdehaan reviewed Dec 5, 2017

View changes

piatra force-pushed the piatra:bug-3766-custom-image-picker branch from 2a7bd74 to d16c19f Dec 6, 2017

sarracini reviewed Dec 14, 2017

View changes

piatra force-pushed the piatra:bug-3766-custom-image-picker branch 3 times, most recently from 2e892e4 to fbbcce1 Dec 19, 2017

piatra force-pushed the piatra:bug-3766-custom-image-picker branch from fbbcce1 to 5063658 Jan 8, 2018

piatra force-pushed the piatra:bug-3766-custom-image-picker branch from 5063658 to 8ff3df9 Jan 9, 2018

ncloudioj mentioned this pull request Jan 15, 2018

Various new fields for Activity Stream mozilla/splice#417

Closed

Mardak reviewed Jan 26, 2018

View changes

piatra force-pushed the piatra:bug-3766-custom-image-picker branch 2 times, most recently from 07420ab to f104567 Jan 30, 2018

Mardak assigned piatra and unassigned Mardak Feb 16, 2018

piatra added 15 commits Mar 5, 2018

Update the style for the custom image trigger

8b0646a

backport the string changes

ae62785

Update tests

5359f8c

rebase issue

2d2039e

Rename method

ff31a73

Refactor condition

8b22acb

refactor customScreenshot property into screenshot

e1d7d6c

Disable the input during a request

05aed44

https://design.firefox.com/photon/patterns/inactive.html

refactor css

36288ed

Editing the custom url after a failed request should revert to the in…

3e0e2b7

…ital image

Move some of the logic for handling failure and success in the Reducer

ff407ae

* TopSites reducer clears screenshotPreview and requestFailed status when necessary * Added action for content process to update its own reducer when a request is canceled

Simultaneous form error states

a278c55

Refactor to use just previewUrl and previewResponse

68fbceb

send separate action to main

d7d40fa

Proper comment

7b4ab9d

piatra force-pushed the piatra:bug-3766-custom-image-picker branch from 8f62da6 to 7b4ab9d Mar 21, 2018

Revert "send separate action to main"

7d4b5d6

This reverts commit d7d40fa.

Mardak approved these changes Mar 22, 2018

View changes

piatra added 3 commits Mar 22, 2018

This change somehow got reverted. Removing it again.

4e240f5

Review comments

f549c31

Update/Add tests

0286b6e

Mardak added PR / R+ with nits and removed PR / Needs work labels Mar 22, 2018

Mardak mentioned this pull request Mar 22, 2018

Fix Bug 1384094 - High CPU. network traffic and memory usage when open Newtab page if you have opened / bookmarked a problematic/ huge game page in the past #4050

Merged

piatra merged commit b69291f into mozilla:master Mar 23, 2018
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

piatra deleted the piatra:bug-3766-custom-image-picker branch Mar 23, 2018

dmose added the pushed-to-pine label Mar 30, 2018

mozilla / activity-stream Archived

Fix Bug 1402654 - Allow custom image / thumbnail for an edited top site #3891

Fix Bug 1402654 - Allow custom image / thumbnail for an edited top site #3891

Conversation

piatra commented Dec 4, 2017 • edited

How it works

UI/UX

Mardak commented Dec 5, 2017

Mardak left a comment

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 5, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 • edited Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

This comment has been minimized.

Mardak Dec 5, 2017 Member

This comment has been minimized.

piatra Dec 7, 2017 Author Collaborator

aaronrbenson commented Dec 5, 2017

This comment has been minimized.

pdehaan Dec 5, 2017 Collaborator

This comment has been minimized.

piatra Dec 8, 2017 Author Collaborator

Mardak commented Dec 7, 2017

piatra commented Dec 7, 2017

k88hudson commented Dec 8, 2017

This comment has been minimized.

sarracini Dec 14, 2017 Contributor

This comment has been minimized.

piatra Dec 14, 2017 Author Collaborator

Mardak commented Dec 21, 2017

piatra commented Dec 21, 2017 • edited

piatra commented Jan 8, 2018 • edited

Mardak commented Jan 9, 2018

piatra commented Jan 9, 2018

Mardak left a comment

Mardak commented Feb 16, 2018

piatra commented Mar 21, 2018 • edited

This comment has been minimized.

Mardak commented on system-addon/content-src/components/TopSites/TopSiteForm.jsx in d7d40fa Mar 21, 2018 • edited

Mardak left a comment

This comment has been minimized.

Mardak Mar 22, 2018 Member

This comment has been minimized.

Mardak Mar 22, 2018 Member

This comment has been minimized.

piatra Mar 22, 2018 Author Collaborator

piatra commented Dec 4, 2017 •

edited

Mardak Dec 5, 2017
Member

Mardak Dec 5, 2017
Member

Mardak Dec 5, 2017
Member

piatra Dec 7, 2017
Author Collaborator

Mardak Dec 5, 2017
Member

piatra Dec 7, 2017
Author Collaborator

Mardak Dec 5, 2017
Member

piatra Dec 7, 2017
Author Collaborator

Mardak Dec 5, 2017
Member

piatra Dec 5, 2017
Author Collaborator

Mardak Dec 5, 2017
Member

piatra Dec 7, 2017
Author Collaborator

Mardak Dec 5, 2017 •

edited

Member

piatra Dec 7, 2017
Author Collaborator

Mardak Dec 5, 2017
Member

piatra Dec 7, 2017
Author Collaborator

Mardak Dec 5, 2017
Member

Mardak Dec 5, 2017
Member

piatra Dec 7, 2017
Author Collaborator

Mardak Dec 5, 2017
Member

piatra Dec 7, 2017
Author Collaborator

pdehaan Dec 5, 2017
Collaborator

piatra Dec 8, 2017
Author Collaborator

sarracini Dec 14, 2017
Contributor

piatra Dec 14, 2017
Author Collaborator

piatra commented Dec 21, 2017 •

edited

piatra commented Jan 8, 2018 •

edited

piatra commented Mar 21, 2018 •

edited

Mardak commented on `system-addon/content-src/components/TopSites/TopSiteForm.jsx` in `d7d40fa` Mar 21, 2018 •

edited

Mardak Mar 22, 2018
Member

Mardak Mar 22, 2018
Member

piatra Mar 22, 2018
Author Collaborator

Mardak Mar 22, 2018
Member

Mardak Mar 22, 2018
Member

Mardak Mar 22, 2018
Member

Mardak Mar 22, 2018
Member

Mardak Mar 22, 2018
Member

Mardak Mar 22, 2018
Member

Mardak Mar 22, 2018
Member