feat(create secret) add file upload support #11
Conversation
elysee15
commented
Aug 25, 2021
•
elysee15
commented
Codecov Report
@@ Coverage Diff @@
## main #11 +/- ##
==========================================
- Coverage 58.13% 56.39% -1.75%
==========================================
Files 43 49 +6
Lines 1070 1158 +88
Branches 55 75 +20
==========================================
+ Hits 622 653 +31
- Misses 373 428 +55
- Partials 75 77 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@elysee15 this looks really good - I like the initiative you took to create a new root page to select the secret type; I think you're right that there need to be two different forms, governed by some navigation function. I think the dropzone you put together is very cool and the file upload works perfectly!
My concern is that it makes the create secret forms feel like two separate applications rather than the same one with the same primary concerns (the only thing that is different is file vs. message). Also, to change from file to message and back you'd have to navigate two times, once back home, then to the form you'd like to use.
My proposal is that instead of a SecretType component, we have the forms be wrapped inside of Tabs. This will make the form feel like one thing and make it easier to switch back and forth between the two with only a single click. What do you think?
Also, I had some comments on the file download; I suppose downloading isn't implemented yet in this PR? Can we make sure another ticket is added for that?
| function encodeFileToBase64(file: File): Promise<string | ArrayBuffer | null> { | ||
| return new Promise((resolve, reject) => { | ||
| const reader = new FileReader(); | ||
| reader.readAsDataURL(file); |
There was a problem hiding this comment.
This function returns the file in the following format:
data:text/csv;base64,Im5ldHdvcmsiLCJhbGlhc2VzIgoiYmNoX2NoYWluIiw=
Note the prefix data:text/csv;base64 -- unfortunately this is not handled by the the CLI program, which expects completely base64 encoded text.
$ whisper fetch [mytoken]
illegal base64 data at input byte 4
We will want the web UI and the CLI to work together seamlessly. This means we either need to:
- Adapt the CLI to read and write data URL format
- Adapt the UI to manage base64 encoded data only without the prefix
My preference is choice 2 since there is a file size limit and this adds extra data. However, this depends on how you intend to to download the file in the future; if the download requires the data URL - then I'm happy to go with choice 1. Let me know your thoughts and we'll create a ticket to manage this.
There was a problem hiding this comment.
I'll verify it and let you know
| function dataURLtoFile(dataurl: string, filename?: string): File { | ||
| const fileName = filename || ""; | ||
| const arr = dataurl.split(","); | ||
| const mime = (arr[0].match(/:(.*?);/) as string[])[1]; |
There was a problem hiding this comment.
I guess the question really to my other comment is do you need the mimetype in the data for the download to function correctly?
There was a problem hiding this comment.
I use the mimetype to set the file format
web/src/routes/index.tsx
Outdated
| <Route path="/maintainance" exact component={MaintainancePage} /> | ||
| {status.status === Status.maintainance ? <Redirect to="/maintainance" /> : null} | ||
| <Route path="/secret/:token" exact component={ShowSecret} /> | ||
| <Route path="/" exact component={CreateSecret} /> | ||
| <Route path="/create-secret" exact component={CreateSecret} /> |
There was a problem hiding this comment.
If we go the tabs route, it would be nice to have the create secret component back to /
| { value: "168h", label: "7 days" } | ||
| ]; | ||
|
|
||
| export { FILE_SIZE, LIFETIME_OPTIONS }; |
There was a problem hiding this comment.
Nice to have these constants in one place!
|
|
||
| const FILE_SIZE = 64 * 1024; | ||
|
|
||
| const StyledDropzone: React.FC<StyledDropzoneProps> = ({ form }) => { |
There was a problem hiding this comment.
I very much like the dropzone for the file upload - nice work!
|
I planned to make a commit for download part after this PR |
|
@elysee15 if we need to hold off on the base64 encoding question until the next PR that's fine with me. Let's just make sure there is a ticket in Monday with commentary. |
elysee15
force-pushed
the
feat/add-file-upload-support
branch
from
fd4222c to
e035434
Compare
elysee15
force-pushed
the
feat/add-file-upload-support
branch
from
e035434 to
b41fc04
Compare
