Skip to content
This repository was archived by the owner on May 22, 2021. It is now read-only.

Favicon now shows visual progress #1269

Closed
wants to merge 13 commits into from
Closed

Favicon now shows visual progress #1269

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
60a6387
favicon progress bar
Mar 20, 2019
6f472ff
made variable name more relevant
Mar 20, 2019
85116c4
removed unsed code
Mar 20, 2019
d433fee
removed unwanted comment
Mar 20, 2019
e56aa55
color changed to #0090ed
Mar 20, 2019
2624b1f
changed 100/100 to 1
Mar 20, 2019
f5334b0
varaible name changed to more meaningful
Mar 20, 2019
6092e4b
converted division to multiplication and calculating radius inside func
Mar 20, 2019
029b0e1
fix var name
Mar 20, 2019
38efe9c
pr comment changes and progress line starts from top instead of right
Mar 21, 2019
98b20e7
constants at the top
Mar 21, 2019
c15a322
query selector fix + revert favicon in case of cancel and complete
Mar 21, 2019
46153ca
Merge branch 'master' into favicon-progressbar
absolutelightning Jun 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/ui/archiveTile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const html = require('choo/html');
const raw = require('choo/html/raw');
const assets = require('../../common/assets');
const faviconProgressBar = require('./faviconProgressbar');
const {
bytes,
copyToClipboard,
Expand Down Expand Up @@ -377,6 +378,7 @@ module.exports.uploading = function(state, emit) {
const progress = state.transfer.progressRatio;
const progressPercent = percent(progress);
const archive = state.archive;
faviconProgressBar.updateFavicon(progressPercent);
return html`
<send-upload-area
id="${archive.id}"
Expand Down
48 changes: 48 additions & 0 deletions app/ui/faviconProgressbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
const { platform } = require('../utils');
const assets = require('../../common/assets');

module.exports.updateFavicon = function(percentageString) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

if (platform() === 'web') {
const percentage = parseInt(percentageString.replace('%', ''));
Copy link
Contributor

@johngruen johngruen Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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)

if (percentage === 0 || percentage === 100) {
const link = document.querySelector("link[rel*='icon']");
link.type = 'image/png';
link.href = assets.get('favicon-32x32.png');
document.getElementsByTagName('head')[0].appendChild(link);
return;
}
const link = document.querySelector("link[rel*='icon']");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

const lineWidth = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const color = '#0090ed';
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly loaderColor would be better here


const span = document.createElement('span');
span.textContent = percentageString;
const context = canvas.getContext('2d');
canvas.width = canvas.height = size;

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

const radius = (size - lineWidth) / 2;

const drawCircle = function(color, lineWidth, percent) {
context.beginPath();
context.arc(0, 0, radius, 0, Math.PI * 2 * percent, false);
context.strokeStyle = color;
context.lineCap = 'square';
context.lineWidth = lineWidth;
context.stroke();
};

const drawNewFavicon = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

drawCircle('#efefef', lineWidth, 1);
drawCircle(color, lineWidth, percentage / 100);
};

drawNewFavicon(link);
link.href = canvas.toDataURL();
document.getElementsByTagName('head')[0].appendChild(link);
}
};
4 changes: 4 additions & 0 deletions app/ui/home.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,21 @@ const { list } = require('../utils');
const archiveTile = require('./archiveTile');
const modal = require('./modal');
const intro = require('./intro');
const faviconProgressbar = require('./faviconProgressbar');
Copy link
Contributor

Choose a reason for hiding this comment

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

this should live in app/controller.js


module.exports = function(state, emit) {
const archives = state.storage.files
.filter(archive => !archive.expired)
.map(archive => archiveTile(state, emit, archive));
let left = '';

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

focus event not triggering. could you help?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, don't worry about the focus event

left = archiveTile.wip(state, emit);
} else {
faviconProgressbar.updateFavicon('0%');
left = archiveTile.empty(state, emit);
}
archives.reverse();
Expand Down