New ui! #191

dnarcese · 2017-07-13T15:41:12Z

Fixes #169 "UX measurement and assets"

pdehaan · 2017-07-13T16:15:05Z

package.json

@@ -14,6 +14,7 @@
    "express-handlebars": "^3.0.0",
    "helmet": "^3.6.1",
    "jquery": "^3.2.1",
+    "jquery-circle-progress": "^1.2.2",


AH! I think you added a dependency to package.json, but didn't update the package-lock.json shrinkwrap file, which may explain why Circle-CI is blowing up.

It works for me locally if I do this:

$ rm -rf node_modules package-lock.json $ npm i && npm run lint

pdehaan · 2017-07-13T16:22:28Z

public/resources/fontello-24c5e6ad/demo.html

@@ -0,0 +1,311 @@
+<!DOCTYPE html>


Not sure if all these files are needed or not. I can't imagine we'd want this on prod. /cc @ericawright

Same applies to the LICENSE and README files above. Not sure if it's good enough(tm) to include the fontello licensing information in the root README.md and not bundle those files in the /public/ dir.

thanks for pointing that out. from the readme: "If your project is open-source, usually, it will be ok to make LICENSE.txt file publicly available in your repository." Se we can remove the readme file, but we need to keep the license file around somewhere.
As for the other files, I believe it is all CSS directly affecting the icons, where else would you propose we put it?

Proposal: Let's remove the following files:

public/resources/fontello-24c5e6ad/LICENSE.txt

public/resources/fontello-24c5e6ad/README.txt

public/resources/fontello-24c5e6ad/demo.html

Then let's copy the contents of public/resources/fontello-24c5e6ad/LICENSE.txt into our root README.md in the Licenses section below our MPL-2.0 notice.

pdehaan · 2017-07-13T16:25:13Z

public/resources/illustration_download.svg

+		</g>
+	</g>
+</g>
+</svg>


I think we have an existing bug for this (Optimize images #63), but we could probably run all these through svgo or imagemin (at checkin or during build time) and shave a few KB.

pdehaan · 2017-07-13T16:30:01Z

views/download.handlebars

+      <div class="progress-bar" id="dl-progress">
+        <div class="percentage">
+          <span class="percent-number"></span>
+          <span class="percent-sign">%</span>


~~Personal curiosity, but couldn't we just do this w/ CSS and save a couple nested <span>s?~~

<div class="percentage">13</div>

.percentage::after { content: "%"; }

Nevermind. I think they are separate because the formatting looks slightly different for each element.

pdehaan · 2017-07-13T16:35:25Z

views/index.handlebars

+        Private, Encrypted File Sharing
+      </div>
+      <div class="description">
+        Send files through a safe, private, and encrypted link that automatically expires to ensure your stuff does not remain online forever.<br> <a href="" class="link">Learn more</a>


Where should "Learn more" link to here?

"needs ux" 😉
maybe we take it out until we have something to link to?

pdehaan · 2017-07-13T16:38:46Z

views/index.handlebars

+      <div class="description">
+        Unfortunately this browser does not support the web technology that powers Firefox Send. You'll need to try another browser. We recommend Firefox!
+      </div>
+      <form action="https://www.mozilla.org/en-US/firefox/new/?scene=2" target="_blank">


Nit: Remove the /en-US/ bit and let mozilla.org handle the locale detection.

pdehaan · 2017-07-13T16:57:17Z

When I try viewing this PR locally, it seems to show all the states on the homepage:
https://screenshots.firefox.com/2qSn57pKiTUoAtSc/localhost

UPDATE: Actually, this may not be a thing. I think I was running npm start locally, instead of npm run dev.

pdehaan · 2017-07-13T17:00:15Z

Also, on Chrome, it doesn't look like the drop zone or table have the same styles applied as Firefoxen:

Figure 1: Chrome stable/release channel

Figure 2: Firefox Nightly channel

abhinadduri · 2017-07-13T17:08:01Z

@pdehaan Same on my end, seems like some of the fonts are not loading properly. Also on the download page, it seems like the progress update is appending instead of overriding (picture attached).

ericawright · 2017-07-13T16:43:57Z

views/index.handlebars

+        Private, Encrypted File Sharing
+      </div>
+      <div class="description">
+        Send files through a safe, private, and encrypted link that automatically expires to ensure your stuff does not remain online forever.<br> <a href="" class="link">Learn more</a>


where should this link to?

ericawright · 2017-07-13T16:58:00Z

frontend/src/upload.js

      }
    }

    // create popup
    popupDiv.classList.add('popup');
    $popupText.html(
-      '<span class="del-file">Delete</span><span class="nvm" > Nevermind</span>'
+      '<span class=\'del-file\'>Delete</span><span class=\'nvm\' > Nevermind</span>'


try `` for the outside, then you don't have to escape the single quotes just to please the linter

I think we can also tweak the ESLint quotes rule to allow escaping of strings:

quotes: [error, single, {avoidEscape: true}]

Or in this case since it's HTML strings, just use double quotes on the internal attributes.

'<span class="del-file">Delete</span> <span class="nvm">Nevermind</span>'

ericawright · 2017-07-13T17:03:28Z

frontend/src/upload.js

@@ -253,32 +293,41 @@ $(document).ready(function() {
        toggleHeader();
      });
    });
+    document.getElementById('delete-file').onclick = () => {


When I click "delete file", it takes me to the home page that lists all the files. at first, my deleted file will be there, then I can see it visually update to remove it. It should be gone from the beginning.

ericawright · 2017-07-13T17:15:12Z

frontend/src/upload.js

  });
-
+  //cancel the upload
+  $('#cancel-upload').click(() => {});


what should happen here? I assume you'll add it later.

ericawright · 2017-07-13T17:16:02Z

frontend/src/upload.js

@@ -44,24 +46,40 @@ $(document).ready(function() {
    document.body.removeChild(aux);
    //disable button for 3s
    $copyBtn.attr('disabled', true);
-    $copyBtn.html('Copied!');
+    $('#link').attr('disabled', true);
+    $copyBtn.html('<span class=\'icon-check\'></span>');


try ` and ' here too

ericawright · 2017-07-13T17:29:51Z

public/main.css

+
+.percent-number {
+  font-size: 43.2px;
+  letter-spacing: -0.78px;


this and font-family can be rules on .percentage to affect both children, then you won't have to write them twice.

ericawright · 2017-07-13T17:31:50Z

public/main.css

+}
+
+.progress-text {
+  font-family: 'SF Pro Text', sans-serif;


same here, some of these rules can pass from .upload to both #cancel-upload and progress-text
it might be worth setting font-family: 'SF Pro Text'; on the body, then only making rules for the ones that are different?

ericawright · 2017-07-13T17:35:28Z

public/main.css

+  font-family: 'SF Pro Text';
+  margin-top: 50px;
+  margin-bottom: 12px;
+  cursor: pointer;


perhaps we make a rule for button and add some default rules to that too, like this one.

ericawright · 2017-07-13T17:39:39Z

frontend/src/upload.js

+    del.innerHTML =
+      '<span class=\'icon-cancel-1\' title=\'Delete\' style=\'margin-left: -7px\'></span>';
+
+    link.innerHTML = '<span class=\'icon-docs\' title=\'Copy URL\'></span>';


Can we get some feedback on clicking this just to show that it has been copied, a slight darkening of the colour, or bolding the font, at a minimum.

ericawright · 2017-07-13T17:47:09Z

@pdehaan I can't replicate the multiple states. Do you have anything that might be affecting it, or anything in the console? it's a known bug on ie, but works perfectly for me on Firefox and Chrome

pdehaan · 2017-07-13T17:47:24Z

Seeing some font issues on the upload result page:

pdehaan · 2017-07-13T17:52:28Z

I can't replicate the multiple states. Do you have anything that might be affecting it, or anything in the console? it's a known bug on ie, but works perfectly for me on Firefox and Chrome

@ericawright I think this was an issue with me running npm start instead of npm run dev.

ericawright · 2017-07-13T17:56:58Z

@pdehaan It still only shows one state when I run npm start

pdehaan · 2017-07-13T18:12:50Z

Anybody else seeing progress bar issues when uploading large files locally? Mine always shows 100%.

ericawright · 2017-07-13T18:19:28Z

@pdehaan that's because it uploads so fast! if you use responsive design mode in the dev tools you can turn on throttling and slow down the upload/download. The animation always has a minimum time that it will take however, so it seems behind the number.

pdehaan · 2017-07-13T19:06:02Z

that's because it uploads so fast! if you use responsive design mode in the dev tools you can turn on throttling and slow down the upload/download

I think regardless of upload speed, I think the displayed percentage should match the progress bar's filled-ness. Not sure if there's an easy way to force that, but that seems stretchy at best and could definitely be done post-merge.

pdehaan · 2017-07-13T19:26:56Z

public/main.css

-  font-weight: 300;
-  font-style: normal;
-  background-size: contain;
+  background: url('resources/Send_bg.svg');


Nit: "send_bg.svg"

pdehaan · 2017-07-13T19:45:07Z

public/main.css

 }

 .upload-window {
-  border: 1px dashed;
+  border: 1px dashed rgb(0, 148, 251, 0.5);


Should this be rgba()?

pdehaan · 2017-07-13T19:45:46Z

public/main.css

-#upload-img {
-  padding-right: 20px;
+.upload-window.ondrag {
+  border: 3px dashed rgb(0, 148, 251, 0.5);


ditto re: rgba()

pdehaan · 2017-07-13T19:48:01Z

public/main.css

-  height: 250px;
-  border-radius: 5px;
+  width: 640px;
+  height: 254.7px;


This feels oddly specific.

pdehaan · 2017-07-13T20:09:28Z

public/main.css

  cursor: pointer;
+  border: none;


356:11 warning A value of `none` is not allowed. `0` must be used border-zero

pdehaan · 2017-07-13T20:15:50Z

public/main.css

 }

 table {
-  table-layout: fixed;
+  border-collapse: collapse;
+  font-family: Segoe UI, 'SF Pro Text', sans-serif;


Does 'Segoe UI' need to be wrapped in single quotes?

Same w/ lines 193 and 200 below. 🤷‍♀️

pdehaan · 2017-07-13T23:06:26Z

frontend/src/download.js

+      $('.progress-text').append(
+        ` (${(progress[0] / 1000000).toFixed(2)}MB of ${(progress[1] /
+          1000000).toFixed(2)}MB)`
+      );


I think this is where the append bug is happening that @abhinadduri was reporting in #191 (comment).

pdehaan · 2017-07-14T00:03:44Z

views/index.handlebars

+      </div>
+      <form action="https://www.mozilla.org/en-US/firefox/new/?scene=2" target="_blank">
+        <button id="dl-firefox" type="submit">
+          <img src="/resources/firefox_logo-only.svg" id="firefox-logo"/>


Nit: Add an alt="" attr, for giggles.

pdehaan · 2017-07-14T00:04:37Z

views/download.handlebars

      </div>
+      <img src="/resources/illustration_download.svg" id="download-img"/>


Nit: Add alt="" attribute.

abhinadduri · 2017-07-14T18:02:52Z

The progress bar for uploads stays at 0 for me until the file download is done, at which point the progress bar quickly goes to 100. @ericawright Maybe the upload was big enough to go past the minimum animation time?

pdehaan · 2017-07-14T22:27:52Z

I think we're also missing some footer links: https://github.com/mozilla/testpilot-assets/blob/master/Send/Send_03_uploading_file/preview/page-1-uploading-file.png

Note the footer links of "Mozilla", "Legal", "About Test Pilot", "Privacy", "Terms", "Cookies", and then "GitHub" and "Twitter" logos.

pdehaan · 2017-07-14T22:43:23Z

frontend/src/download.js

-      );
+      if (progress[1] < 1000000) {
+        $('.progress-text').html(
+          `${filename} (${(progress[0] / 1000).toFixed(1)}KB of ${(progress[1] / 1000).toFixed(1)}KB)`


I think I'm getting a bit lost with all these magic numbers floating around. Not sure if we want to create some consts for 1000 (KB), 1000000 (MB), and 1000000000 (GB).

Something like:

const KB = 1000; const MB = KB * 1000; const GB = MB * 1000; if (progress[1] < MB) { $('.progress-text').html( `${filename} (${(progress[0] / KB).toFixed(1)}KB of ${(progress[1] / KB).toFixed(1)}KB)` ); else if (progress[1] < GB) {

Not sure if there'd be a prettier way to do something like:

function toKB(value) { return `${(value / KB).toFixed(1)}KB`; } function toMB(value) { return `${(value / MB).toFixed(1)}MB`; } function toGB(value) { return `${(value / GB).toFixed(1)}GB`; } $('.progress-text').html(`${filename} (${toKB(progress[0])} of ${toKB(progress[1])}`);

pdehaan · 2017-07-14T22:54:43Z

The root .eslintrc.yml will need to tweak the quotes rule to allow for escaping. Not sure if Prettier or something is wrapping the single quotes in double quotes for readability during npm run format (it's much better than the escaped single quotes version), but that's what seems to be causing a few of the ESLint errors in Circle-CI:

quotes: [error, single, {avoidEscape: true}]

dannycoates · 2017-07-17T22:35:29Z

Awesome work @dnarcese!!!

Let's merge now and do follow-up PRs for anything left.

new ui

9032e42

dnarcese requested a review from ericawright July 13, 2017 15:41

lint

fcdb905

dnarcese force-pushed the ui branch from ee0f22b to fcdb905 Compare July 13, 2017 15:44

pdehaan reviewed Jul 13, 2017

View reviewed changes

ericawright suggested changes Jul 13, 2017

View reviewed changes

pdehaan reviewed Jul 13, 2017

View reviewed changes

public/main.css Outdated

height: 250px;

border-radius: 5px;

width: 640px;

height: 254.7px;

Copy link

Collaborator

pdehaan Jul 13, 2017

Choose a reason for hiding this comment

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

This feels oddly specific.

pdehaan reviewed Jul 13, 2017

View reviewed changes

pdehaan reviewed Jul 14, 2017

View reviewed changes

dnarcese force-pushed the ui branch 4 times, most recently from dec7a0b to 20afd7a Compare July 17, 2017 18:40

fixing things

61c49fb

dnarcese force-pushed the ui branch from 20afd7a to 61c49fb Compare July 17, 2017 20:30

dannycoates merged commit 5ce0846 into master Jul 17, 2017

pdehaan mentioned this pull request Jul 17, 2017

Pretty print file sizes? #198

Closed

		</div>
		<img src="/resources/illustration_download.svg" id="download-img"/>

New ui! #191

New ui! #191

Conversation

dnarcese commented Jul 13, 2017 • edited by pdehaan Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericawright Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdehaan Jul 13, 2017 • edited Loading

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

pdehaan commented Jul 13, 2017 • edited Loading

pdehaan commented Jul 13, 2017

abhinadduri commented Jul 13, 2017

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

ericawright commented Jul 13, 2017

pdehaan commented Jul 13, 2017

pdehaan commented Jul 13, 2017

ericawright commented Jul 13, 2017

pdehaan commented Jul 13, 2017

ericawright commented Jul 13, 2017 • edited Loading

pdehaan commented Jul 13, 2017 • edited Loading

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

abhinadduri commented Jul 14, 2017

pdehaan commented Jul 14, 2017

Choose a reason for hiding this comment

pdehaan commented Jul 14, 2017

dannycoates commented Jul 17, 2017

dnarcese commented Jul 13, 2017 •

edited by pdehaan

Loading

ericawright Jul 14, 2017 •

edited

Loading

pdehaan Jul 13, 2017 •

edited

Loading

pdehaan commented Jul 13, 2017 •

edited

Loading

ericawright commented Jul 13, 2017 •

edited

Loading

pdehaan commented Jul 13, 2017 •

edited

Loading