factored out progress into progress.js by dannycoates · Pull Request #457 · mozilla/send

dannycoates · 2017-08-06T16:04:19Z

Fixes #349
Fixes #366
Fixes #462

dannycoates · 2017-08-06T16:08:40Z

frontend/src/fileReceiver.js

      })
      .then(([fdata, key]) => {
-        this.emit('decrypting', true);
+        this.emit('decrypting');


Turns out we don't really need the start/finish part of these events. Removing the finished case simplifies the listeners.

Although I don't think it affects the outcome of the tests, I think we should change the frontend tests to reflect that these listeners only get called once.

dannycoates · 2017-08-06T16:18:56Z

public/locales/en-US/send.ftl

    .title = Download
 downloadNotification = Your download has completed.
 downloadFinish = Download Complete
+fileSizeProgress = ({ $partialSize } of { $totalSize })


This PR adds a couple strings for l10n, but doesn't use them yet. @flodolo, is it a good idea to pre-stage strings so they can be translated before using them, or better just to use them now?

In fileSizeProgress case it's a new string previously not localized, whereas uploadingPageProgress will replace the uploadingPageHeader to be symmetric with downloadPageProgress.

In general it's not a good idea to preland strings, because a) you won't have any way to test it and b) you might have to change your mind in the implementation phase.

Note: if this PR makes a string obsolete, that string should be removed.

dannycoates · 2017-08-06T16:23:04Z

frontend/src/download.js

+$(() => {
+  gcmCompliant()
+    .then(() => {
+      $('#download-btn').on('click', download);


bumped download() up to make the "main" function cleaner.

dannycoates · 2017-08-06T19:48:51Z

now also fixes #349

dannycoates · 2017-08-06T19:49:42Z

and fixes #366

pdehaan · 2017-08-07T05:26:31Z

Off-topic, but I think the "Fixes #___" only auto-closes the referenced bugs if you include it in the commit message or original PR comment.

pdehaan · 2017-08-07T05:30:21Z

frontend/src/utils.js

+  typeof Intl.NumberFormat === 'function'
+);
+
+const UNITS = ['B', 'kB', 'MB', 'GB'];


Does this work for l10n? Is a "MB" the same in English, German, Japanese, Russian, etc?
/cc @flodolo @mathjazz

We localize them, and you don't even need to go to a different script
https://transvision.mozfr.org/string/?entity=toolkit/chrome/mozapps/downloads/downloads.properties:megabyte&repo=central

Having said that, I wonder if we have access to them through Intl API @zbraniecki would know for sure.

Side note: kB is not going to be consistent with Firefox en-US (don't get me started)
https://hg.mozilla.org/mozilla-central/file/default/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties#l67

Unfortunately, the https://github.com/zbraniecki/proposal-intl-unit-format has not been implemented yet in ECMA402 :(

pdehaan · 2017-08-07T05:37:12Z

frontend/src/utils.js

+    ? n.toLocaleString(navigator.language, {
+        minimumFractionDigits: 1,
+        maximumFractionDigits: 1
+      })


Sorry, I've always struggled to understand localization...
Not sure I understand how this is working... What happens here if I have a local navigator.language of some non-supported localization on stage/prod (like Hebrew, for example), but I get a match on my second or third choice l20n fallback (such as Japanese)? Would the numbers be displayed in my preferred Hebrew language format, but the surrounding text be Japanese because that is one of the official production availableLanguages? Or am I totally missing something?

@zbraniecki or @stas
Any advice?

First of all, never use navigator.language, use navigator.languages - this is a fallback chain of your preferred languages.

Now, what do you mean by "my second or third choice l20n fallback"? If I understand correctly this is a website, so it will get navigator.languages as a fallback chain from the user.
You just have to pass it to toLocaleString and it'll pick the best locale matching the list.

@zbraniecki Sorry, I was trying to explain that I only speak Hebrew, Japanese, and Bulgarian:

I wasn't sure if this meant that my file sizes would be in Hebrew (which is not an available language for Send), but the rest of the site would be Japanese (or Bulgarian, if Japanese wasn't an available language for Firefox Send site + l20n).

I wasn't sure if this meant that my file sizes would be in Hebrew (which is not an available language for Send), but the rest of the site would be Japanese (or Bulgarian, if Japanese wasn't an available language for Firefox Send site + l20n).

Ah, yes. If you pass navigator.languages it will be ["he", "ja", 'bg"]. Since all browsers carry NumberFormat data for he, we'll format into he.

If you want to link the raw Intl API to use the same languages as were negotiated for l20n, you need to retrieve the result of language negotiation for l20n.

I'm not sure how this API currently works, but it should be something along the lines of document.l10n.locales which should be the list of locales that l20n negotiated down to.
Since he is not available, the list would end up being sth like ['ja', 'bg', 'en-US'] assuming both ja and bg are available, and en-US is the last fallback.

In that case, passing that list fo the Intl API will do the right thing for you.

I haven't been working with l20n.js package in a while, so CC'ing @stasm, who's maintaining it - he will hopefully be able to point out how to retrieve the negotiated locale list from l20n to pass to the Intl API.

Sorry, I missed the notification for this. l20n.js exposes the currently negotiated languages in the langs attribute of the <html> element. You can access it with document.documentElement.getAttribute('langs'). It's a space-separated list of local codes.

pdehaan · 2017-08-07T05:44:24Z

views/download.handlebars

+            data-size="{{sizeInBytes}}"
+            data-ttl="{{ttl}}"
            data-l10n-id="downloadFileName"
            data-l10n-args='{"filename": "{{filename}}"}'></span>


Unrelated to this glorious PR, but looking at it with my special eyes, I think this is a bug...
I can seemingly break the download page hard on production by uploading a file with a pair of double quotes in the filename. Not sure if we can encode the filename here (and on line 6 above, and possibly elsewhere, or whatever).

UPDATE: Filed as #462, "File downloads break if you have a double quote in the filename".

Nice catch @pdehaan. We ought to be doing data-l10n-args='{{JSON.stringify({filename})}}'

Possibly a few places:

$ git grep -n "{{filename}}" views/download.handlebars:7: data-l10n-args='{"filename": "{{filename}}"}'></span> views/download.handlebars:23: data-l10n-args='{"filename": "{{filename}}", "size": "{{filesize}}"}'> views/download.handlebars:34: <div class="progress-text">{{filename}}</div>

dannycoates requested a review from flodolo as a code owner August 6, 2017 16:04

dannycoates commented Aug 6, 2017

View reviewed changes

dannycoates force-pushed the refactor-progress branch 2 times, most recently from 87c56bb to 6784bad Compare August 6, 2017 19:46

dannycoates force-pushed the refactor-progress branch 2 times, most recently from 0bfc572 to 671cfc2 Compare August 7, 2017 00:06

pdehaan reviewed Aug 7, 2017

View reviewed changes

factored out progress into progress.js

c91d24c

dannycoates force-pushed the refactor-progress branch from 671cfc2 to c91d24c Compare August 7, 2017 06:45

abhinadduri approved these changes Aug 7, 2017

View reviewed changes

dannycoates merged commit d2b5703 into master Aug 7, 2017

pdehaan deleted the refactor-progress branch August 10, 2017 16:17

Conversation

dannycoates commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycoates commented Aug 6, 2017

Uh oh!

dannycoates commented Aug 6, 2017

Uh oh!

pdehaan commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdehaan Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdehaan Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dannycoates commented Aug 6, 2017 •

edited

Loading

pdehaan Aug 7, 2017 •

edited

Loading

pdehaan Aug 7, 2017 •

edited

Loading