Add file upload functionality #145
Conversation
There was a problem hiding this comment.
Thanks! Indeed this needs some work, please test it with large files. https://github.com/45Drives/cockpit-navigator/ does file uploads in chunks. It uses a spawn channel with .input() for that, instead of fsreplace -- the latter does not have a chunked API indeed, and is only meant for small files. That spawn based approach was recently fixed and properly supported in cockpit-project/cockpit#19325
MahmoudHamdy02
force-pushed
the
file-upload
branch
from
2ef1221
to
9e866a6
Compare
MahmoudHamdy02
force-pushed
the
file-upload
branch
from
438cd96
to
9023f67
Compare
|
Hi @garrett, I need some help with the design here :) I've added a progress bar in the header, here is the initial implementation for now: Where should it be located? Thanks! Edit: Also, where should the |
|
Do we want to only allow one upload at a time? (I see the disable upload button when uploading.) Because that would considerably change the how the UI would be handled if it supports multiple uploads or just one. If multiple, we probably would want:
This is similar to how GNOME Files handles long file operations (such as large files, network copying or moving, etc.): In the actual implementation, they've dropped the number and show it like this:
It's similar to how both Firefox and now Chrome handle this too.
(Just like in Nautilus, there isn't a visible button unless you hover or click.) We would probably want this upload indicator to go to the right of the download button (as it's conceptually tied to the upload action), after an upload is started. It'd cause a shift when it appears, but this is after uploading via a modal (or drag and drop). |
|
If we do only want to allow only one upload, then we would replace the upload button with an indicator with the same width. It would change from [ Upload ] to ◕ Uploading... Where ◕ is a circular progress indicator, and the button wouldn't be a button any longer. The downside (other than only supporting one upload at a time) is that this doesn't allow you to cancel the operation. It would be similar to the secondary loading "progress" button, but with an actual state instead of just a spinner (so it would actually show progress):
|
|
The bad news for either implementation is that PatternFly does not provide all the widgets we need in either case. There are donut utilization charts, but they're overkill for what we need. And if we went with the one file upload at a time method (as a first pass, I guess?) then we still wouldn't be able to properly implement it in the button. https://www.patternfly.org/charts/donut-utilization-chart |
|
Meanwhile, I've made a custom HTML+CSS element that does what we want in this instance: https://codepen.io/garrett/pen/oNmWxRp
It's all controlled by the So you'd want to use that element with that CSS. Set the colors to PF colors and adjust the I tried a number of different methods, including SVGs, using dash offset, and more... this seemed like the simplest way to implement it. The downside is as they're both at the same % (which is a string, BTW), they don't have anti-aliasing, so the edge looks a bit jagged when larger. It really is fine though at a smaller size... I've checked. The height @ 100% makes it fill up the vertical space. We could change that to a set height if we wanted... but I thought it would be most flexible like this, at least in this demo. |
| --progress-fill: grey; | ||
| --progress-empty: lightgrey; |
There was a problem hiding this comment.
Make sure to set these variables to PF colors! Thanks!
Instead of relying on dd and moving temporary files ourself the bridge can already do this for us. This also uses the progress api to update the progress bar and paves the way for multi file uploads.
|
I've rebased this PR and updated it to use the We can support multiple file uploads, the code in theory should already work except for the progress circle maybe. This is how the progress circle looks like currently:
Some notable issues:
|
|
The plan is:
When the upload starts: Add beforeUnloadNavigator https://codepen.io/garrett/pen/ZEwJPdp (We want the middle one, not the pie chart) |
| let len = 0; | ||
| const content = readerEvent.target.result; | ||
| console.log(content); | ||
| len = content.byteLength; |
There was a problem hiding this comment.
https://stackoverflow.com/a/25813769
This might read the whole file in memory.
|
I thought about this today when thinking of other icons in Navigator: Something even easier is probably just using the upload icon (
(We'd have to revisit when implementing multiple uploads though. But we'd have to anyway.) |
|
Good news, I have a local branch with working upload with Screencast.from.2024-03-08.12-06-07.webm@garrett I have two issues so far with implementing the discussed button:
@allisonkarlitskaya we need to pick up our discussion on matrix:
|
Some updates from our discussion this morning:
|
Yes. Why not? |
I was thinking that this might lead to some blocking / a lot of pings? Performance issues? |
|
If we're thinking of adding cancelling, we should commit to the fully featured upload widget, as we'd have to completely change functionality of the button (simple would have to have click for canceling, but then it'd be repurposed to the near-opposite of adding another upload... so that would be confusing and would break expectations). Here's a design for the full version. Let's see if the scope creep between this PR and the full thing would be too much or not to implement it all in one go. Firstly, this assumes the default current mockup of:
Note: There is no icon for upload in this version, as it would not need a placeholder as it wouldn't get a spinner inside the button.
Uploading a file would cause a spinner to appear after a short while (to prevent uploading a tiny file to cause page shift... so probably like > 1s... we'll have to figure out what "feels" right). When clicking on it, you'd get a popover (https://www.patternfly.org/components/popover/) with the filename, progress, and remove actions. They're trash icons for remove, so they're not confused with the typical (X) icon to close a popover. Each entry has a border at the top to separate it from previous or the header. This makes it clearer which item the trash acts upon. Unfortunately, this would cause a shift in the page for the filter and view actions. I don't think that can be helped, unless we swap the order of upload and the view. However, the convention is to have view first, then actions on the right. And the ⋮ menu is always on the right as part of the actions group, so this design is correct, even if it causes a shift in the view. But this is also why there's a small delay and we would only show the status for long-running uploads. Additionally, the spinner is to the left of the upload button instead of the right, as we wouldn't want to shift the upload button when someone is uploading multiple files, as this could cause a misclick. When the file is finished uploading, it should show up in the icon or details view with a yellow new highlight that fades out (our typical new animation). Additionally, when the file is added, the view should not scroll (this is probably the default anyway; I'm just clarifying it here). We'd additionally want to support drag and drop, but that should be active on at least the view area (but probably actually on the full page) with an event handler and a custom overlay thing which has a scrim (either to dim the page or to hide it) and a message about it being a drop target. But I'd consider drag and drop to be fine for a follow-up, even if we're doing the upload implementation and widgetry in a "full" state in this PR. |
|
Here's the current version of the Penpot file (in a ZIP, due to GitHub upload restrictions). Upload is on the upload page. (There are old mockups and new mockups in the file, so tread lightly if you're using this and don't expect any of the mockups to be final.) |
|
Idea from this morning, if a user has IdleTimeout enabled we should probably use a session inhibitor so that uploads don't logout the users session. https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.login1.html |
|
@jelly: Additionally, while uploads are happening, we should block tab/window closing using beforeunload https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event (which should be reset when all of the the uploads are complete) |
|
Latest snapshot of the Penpot document, BTW: (There's a possibility that there are Penpot 2.0 related issues still; I had a few buttons that said "Secondary" instead of the actual text, for example. I think I fixed it all.) |
|
Superseeded by #384 |
Fixes #31
To do:
beforeunloadeventb.upload_file()beforeunloadhandler during uploads, to prevent the page from being closed, reloaded, navigated away from, etc.