Feature/8166 - Show current upload speed and upload details in tooltip #8187
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
|
|
||
| // "bytesTotal" is a wrong type documentation, | ||
| // fix in progress via https://github.com/transloadit/uppy/pull/4263 | ||
| this.uploadSpeed = getSpeed({ |
There was a problem hiding this comment.
I had the content of getSpeed copied to a local method while figuring out why it wouldn't do what I expected it to, using it directly still returns NaN and breaks the tooltip so further investigation needed/potentially a type error somewhere
There was a problem hiding this comment.
upstream fix has been released.
There was a problem hiding this comment.
Ah nice, thanks for the reminder! I'll see if I can get this PR done this week then :)
| return '' | ||
| } | ||
| const todo = filesize(this.bytesUploaded) | ||
| const done = filesize(this.bytesTotal) |
There was a problem hiding this comment.
Naming of these two variables seems off
There was a problem hiding this comment.
Updated, thx! (no clue what I was thinking 😶🌫️ )
pascalwengerter
force-pushed
the
feature/8166
branch
from
57d5e0c to
da6d967
Compare
|
@JammingBen @kulmann could someone give this a look and potentially test it on oC10 also? Don't think I'll find more time to invest into this, but so far it was looking okay (limited connection via network tab in dev tools, obviously)
|
There was a problem hiding this comment.
The tooltip feels very jumpy because of the rapid text changes and a non-monospaced font. I don't think there is much we can do about it... one thing you could try is using only 1 decimal instead of 2. And the other option would be debouncing the text changes by like 200-500ms, so that the changes are frequent but not as frequent as they are now. I would definitely like to keep the upload speed in the tooltip. Maybe we should indicate somehow that a tooltip is available in this case...
| @@ -0,0 +1,8 @@ | |||
| # Show upload speed | |||
There was a problem hiding this comment.
| # Show upload speed | |
| Enhancement: Show upload speed |
| const totalBytes = filesize(this.bytesTotal) | ||
| const currentUploadSpeed = filesize(this.uploadSpeed) | ||
| // needs $gettext interpolation | ||
| return `${uploadedBytes} of ${totalBytes} (${currentUploadSpeed}/s)` |
There was a problem hiding this comment.
that's a TODO that should be solved before merging. :-) fyi, in the vue3 version of gettext the interpolation happens directly in $gettext (grep for examples in the code)
There was a problem hiding this comment.
Updated, thx
|
Forgot to say, oc10 works fine as well 👍 |
pascalwengerter
force-pushed
the
feature/8166
branch
from
da6d967 to
6115c7b
Compare
|
SonarCloud Quality Gate failed. |
|
I've updated this PR to use |
The "level of jumpiness" is much better now with less decimal places. IMO sufficient without the debounce.
It's good that the tooltip is not accessible for screen readers anyway at the moment. Don't see that this is valuable for screen reader in the current state. A hidden announcer could be an option, but the information is so "nice to have" that I wouldn't try that at the moment. |
Description
Quick draft for #8166, happy to have someone else pick up the ball here :)
Related Issue
Motivation and Context
Community request
Screenshots (if appropriate):
Types of changes
Checklist: