Favicon now shows visual progress #1269

ghost · 2019-03-20T18:39:54Z

Fixes #803 for the web.
Please review.

johngruen · 2019-03-20T19:03:45Z

Gonna check this out now

johngruen · 2019-03-20T19:13:08Z

app/ui/faviconProgressbar.js

+    const size = 32;
+
+    const lineWidth = 5;
+    const color = '#339BFF';


could we make this #0090ed

johngruen · 2019-03-20T19:13:58Z

app/ui/faviconProgressbar.js

+    };
+
+    const drawNewFavicon = function() {
+      drawCircle('#efefef', lineWidth, 100 / 100);


last param could just be 1 here

johngruen · 2019-03-20T19:32:28Z

app/ui/faviconProgressbar.js

+    const canvas = document.createElement('canvas');
+    const size = 32;
+
+    const lineWidth = 5;


I don't like that you're using lineWidth here and in the function as a parmeter. This variable name should be more specific...consider loaderWidth

johngruen · 2019-03-20T19:32:48Z

app/ui/faviconProgressbar.js

+    const size = 32;
+
+    const lineWidth = 5;
+    const color = '#0090ed';


similarly loaderColor would be better here

johngruen · 2019-03-20T19:34:46Z

app/ui/faviconProgressbar.js

+
+module.exports.updateFavicon = function(percentageString) {
+  if (platform() === 'web') {
+    const percentage = parseInt(percentageString.replace('%', ''));


seems like you could clean this up a bit if you used let on this line and then maybe made the variable name a bit more clear by calling it progress instead of percentage since you're already using percent elsewhere

let progress = parseInt(percentageString.replace('%', '')); progress = progress * .01;

then on line 41 you can do drawCircle(color, lineWidth, progress)

johngruen · 2019-03-20T19:43:23Z

app/ui/faviconProgressbar.js

+
+    context.translate(size / 2, size / 2);
+
+    const radius = (size - loaderWidth) / 2;


last thing from me...recommend you calculate radius inside your function since it's dependent on another property of the circle....this means you'll need to pass size into the function as well...

const drawCircle = function(color, lineWidth,OuterWidth,percent) { const radius = (outerWidth - loaderWidth) / 2; ... }

yeah i messed that up...should be outerwidth - lineWidth

johngruen · 2019-03-20T19:45:10Z

@avidyut1 this looks good to me...only change i might make is replacing all division with multiplication (just a preference)...Otherwise the patch works great! I'll defer to @dannycoates for a final review, but from a UX standpoint I'm happy.

ghost · 2019-03-20T19:51:08Z

thanks @johngruen

johngruen · 2019-03-20T19:56:01Z

app/ui/faviconProgressbar.js

-
-    const drawCircle = function(color, lineWidth, percent) {
+    const drawCircle = function(color, lineWidth, outerWidth, percent) {
+      const radius = (outerWidth - loaderWidth) * 0.5;


outerWidth - lineWidth

ghost · 2019-03-20T20:02:33Z

@johngruen integration_tests failed, how to rerun it?

johngruen · 2019-03-20T20:15:54Z

looks like it's good now!

dannycoates

This looks nice! Can you also make the progress line start at the top instead of the right?

dannycoates · 2019-03-21T16:38:50Z

app/ui/faviconProgressbar.js

+const { platform } = require('../utils');
+const assets = require('../../common/assets');
+
+module.exports.updateFavicon = function(percentageString) {


this function should take the raw progressRatio number instead of the string. The string is localized and won't always have the '%' character in it. Instead use the percent function from utils.js to set the text content.

i see percent function may or may not return string with %
so i am using state.transfer.progressRatio is it ok?

dannycoates · 2019-03-21T16:41:23Z

app/ui/faviconProgressbar.js

+
+    context.translate(size * 0.5, size * 0.5);
+
+    const drawCircle = function(color, lineWidth, outerWidth, percent) {


style nit: please use the form function drawCircle( instead of const drawCircle = function(

dannycoates · 2019-03-21T16:53:22Z

app/ui/home.js

@@ -3,17 +3,21 @@ const { list } = require('../utils');
 const archiveTile = require('./archiveTile');
 const modal = require('./modal');
 const intro = require('./intro');
+const faviconProgressbar = require('./faviconProgressbar');


this should live in app/controller.js

dannycoates · 2019-03-21T16:59:46Z

app/ui/home.js

  if (state.uploading) {
    left = archiveTile.uploading(state, emit);
  } else if (state.archive.numFiles > 0) {
+    faviconProgressbar.updateFavicon('0%');


The only calls to updateFavicon should be in controller.js inside updateProgress and the focus event handler (to reset the icon to default).

focus event not triggering. could you help?

ok, don't worry about the focus event

dannycoates · 2019-03-21T17:07:48Z

app/ui/faviconProgressbar.js

+      return;
+    }
+    progress = progress * 0.01;
+    const link = document.querySelector("link[rel*='icon']");


you can bump this up higher and remove the other const link

dannycoates · 2019-03-21T17:16:02Z

app/ui/faviconProgressbar.js

+      context.stroke();
+    };
+
+    const drawNewFavicon = function() {


Try to encapsulate the canvas and other drawing code in this function and return the dataURL. Then you can lift the definition out of updateFavicon and call something like:

const link = document.querySelector("link[rel*='icon']"); link.href = drawNewFavicon(progress);

ghost · 2019-03-21T18:16:24Z

progress bar starts from top instead of right

dannycoates

Almost done. We need to reset the favicon when the upload/download completes or is cancelled. controller.js is probably the right place to put it.

app/ui/faviconProgressbar.js

ghost · 2019-03-21T22:22:57Z

@dannycoates @johngruen thanks for the review, got to learn a lot.

ghost · 2019-04-20T15:25:18Z

when will we merge this PR?

Thynix · 2019-06-08T03:28:11Z

Looks like the codebase changed underneath this and there are merge conflicts now.

Thynix · 2019-06-14T07:14:23Z

Looks like the test failure is linting failing on a file not modified in this PR, so that seems fine?

@dannycoates Does this need any more changes before it can be merged? The open change request to reset the favicon in controller.js is already addressed.

favicon progress bar

60a6387

ghost mentioned this pull request Mar 20, 2019

Hack the favicon to show visual progress #803

Open

Ashesh Vidyut added 3 commits March 21, 2019 00:12

made variable name more relevant

6f472ff

removed unsed code

85116c4

removed unwanted comment

d433fee

johngruen reviewed Mar 20, 2019

View reviewed changes

Ashesh Vidyut added 2 commits March 21, 2019 00:44

color changed to #0090ed

e56aa55

changed 100/100 to 1

2624b1f

johngruen reviewed Mar 20, 2019

View reviewed changes

varaible name changed to more meaningful

f5334b0

johngruen reviewed Mar 20, 2019

View reviewed changes

converted division to multiplication and calculating radius inside func

6092e4b

johngruen reviewed Mar 20, 2019

View reviewed changes

fix var name

029b0e1

dannycoates suggested changes Mar 21, 2019

View reviewed changes

Ashesh Vidyut added 2 commits March 21, 2019 23:37

pr comment changes and progress line starts from top instead of right

38efe9c

constants at the top

98b20e7

dannycoates suggested changes Mar 21, 2019

View reviewed changes

app/ui/faviconProgressbar.js Outdated Show resolved Hide resolved

query selector fix + revert favicon in case of cancel and complete

c15a322

Merge branch 'master' into favicon-progressbar

46153ca

arzzen approved these changes Sep 6, 2019

View reviewed changes

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Favicon now shows visual progress #1269

Favicon now shows visual progress #1269

ghost commented Mar 20, 2019

johngruen commented Mar 20, 2019

johngruen Mar 20, 2019

ghost Mar 20, 2019

johngruen Mar 20, 2019

ghost Mar 20, 2019

johngruen Mar 20, 2019

johngruen Mar 20, 2019

johngruen Mar 20, 2019 •

edited

Loading

johngruen Mar 20, 2019

johngruen Mar 20, 2019

johngruen commented Mar 20, 2019

ghost commented Mar 20, 2019

johngruen Mar 20, 2019

ghost commented Mar 20, 2019

johngruen commented Mar 20, 2019

dannycoates left a comment

dannycoates Mar 21, 2019

ghost Mar 21, 2019

dannycoates Mar 21, 2019

dannycoates Mar 21, 2019

dannycoates Mar 21, 2019

dannycoates Mar 21, 2019

ghost Mar 21, 2019

dannycoates Mar 21, 2019

dannycoates Mar 21, 2019

dannycoates Mar 21, 2019

ghost commented Mar 21, 2019

dannycoates left a comment

ghost commented Mar 21, 2019 •

edited by ghost

Loading

ghost commented Apr 20, 2019

Thynix commented Jun 8, 2019

Thynix commented Jun 14, 2019 •

edited

Loading


		context.translate(size / 2, size / 2);

		const radius = (size - loaderWidth) / 2;


		context.translate(size * 0.5, size * 0.5);

		const drawCircle = function(color, lineWidth, outerWidth, percent) {

Favicon now shows visual progress #1269

Favicon now shows visual progress #1269

Conversation

ghost commented Mar 20, 2019

johngruen commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngruen Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngruen commented Mar 20, 2019

ghost commented Mar 20, 2019

Choose a reason for hiding this comment

ghost commented Mar 20, 2019

johngruen commented Mar 20, 2019

dannycoates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 21, 2019

dannycoates left a comment

Choose a reason for hiding this comment

ghost commented Mar 21, 2019 • edited by ghost Loading

ghost commented Apr 20, 2019

Thynix commented Jun 8, 2019

Thynix commented Jun 14, 2019 • edited Loading

johngruen Mar 20, 2019 •

edited

Loading

ghost commented Mar 21, 2019 •

edited by ghost

Loading

Thynix commented Jun 14, 2019 •

edited

Loading