File Uploader#2013
Conversation
karriebear
force-pushed
the
file-uploader
branch
5 times, most recently
from
046f908 to
4901905
Compare
karriebear
force-pushed
the
file-uploader
branch
2 times, most recently
from
977ccf4 to
2a7a575
Compare
karriebear
force-pushed
the
file-uploader
branch
from
2a7a575 to
7b7bfa6
Compare
karriebear
force-pushed
the
file-uploader
branch
from
7b7bfa6 to
50b0fa3
Compare
karriebear
force-pushed
the
file-uploader
branch
17 times, most recently
from
452be3b to
16513d0
Compare
frontend/src/components/widgets/FileUploader/FileDropzoneInstructions.test.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/widgets/FileUploader/FileDropzoneInstructions.test.tsx
Outdated
Show resolved
Hide resolved
kantuni
left a comment
kantuni
left a comment
There was a problem hiding this comment.
Marvelous job! 🎸
P.S. Few comments (mostly nitpicking).
|
|
||
| const multipleFiles: boolean = element.get("multipleFiles") | ||
| private delete = (fileId: string): void => { | ||
| const file = this.state.files.find(file => file.id === fileId) |
There was a problem hiding this comment.
This will mutate the state as the file is returned as a reference. Instead copy the whole array first, then change the individual value. And again you would have to use setState((prevState) => {}) way of setting the state.
There was a problem hiding this comment.
Yup but here the mutation wouldn't trigger a component update and since we update the state right below, the mutation becomes outdated.
I don't think the function to update state is not necessary given that you can only delete one file at a time. There shouldn't be conflicting updates occurring.
I'm definitely an over optimizer so I find it unnecessary to do the above but can do so if it's thats our preference. Let me know!
There was a problem hiding this comment.
From React documentation:
Because
this.propsandthis.statemay be updated asynchronously, you should not rely on their values for calculating the next state.
I have spent too many hours debugging bugs caused by passing an object to setState.
Thus, I have a very strong opinion on this matter!
| } | ||
| private removeFile = (fileId: string): void => { | ||
| const filteredFiles = this.state.files.filter(file => file.id !== fileId) | ||
| this.setState({ |
| ) | ||
|
|
||
| return { files } | ||
| }) |
There was a problem hiding this comment.
@kmcgrady, I would disagree. IMO, it's the correct way of setting the state here.
| } | ||
|
|
||
| public onNext = (): void => { | ||
| this.setState({ |
There was a problem hiding this comment.
This should use the updater function.
frontend/src/lib/FileHelper.ts
Outdated
| unit: FileSizes, | ||
| rounding = 1 | ||
| ): string => { | ||
| if (!unit) unit = FileSizes.Byte |
There was a problem hiding this comment.
Any strong feelings with regards to one-line if statements?
There was a problem hiding this comment.
nope, but if you do, we should put it as a linting rule instead
There was a problem hiding this comment.
I do. It does look a lot cleaner to me. I'll add a task to the infra board.
karriebear
force-pushed
the
file-uploader
branch
2 times, most recently
from
d51d2d4 to
b2b8821
Compare
karriebear
force-pushed
the
file-uploader
branch
from
b2b8821 to
1bfb9bc
Compare
|
Awesome, work here are some bugs/UX challenges I observed. Visual cutoff of error Single File Upload doesn't indicate single file, especially when multiple rejected files exist. Multiple of the same file combined with rejected files is confusing. My preference for single file is successful file first, followed by rejected in alphabetical order. Multiple files either in alphabetical order all together or successful in upload order followed by rejected in alphabetical order Icon Buttons do not have hover/active/focus states. Single File Uploader, removing the valid used file does not use the next valid file. Display order does not match execution order. |
| color: $primary; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Not a big deal, but disabled is not really used here. Probably not a use case right now, but one to note.
kmcgrady
left a comment
kmcgrady
left a comment
There was a problem hiding this comment.
Changes to address challenges are in the works. Most issues are minor UX challenges that present a longer discussion. To merge in a feature branch, this is functional.
* File Uploader (#2013) * Copying over * Linting * Missed copy - Hashing + Media support * Sidebar UX cleanup * Testing cleanup * linting + cleanup * E2E * linting * Fix unit tests * Get snapshots * Styling cleanup * File sizing + state updating cleanup * File sizing + state updating cleanup * button cleanup + file deletion * Separate dropzone instructions into a new file * Separate dropzone instructions into a new file * Linting * Small/Error text styled componenting * a11y uploaded files * Spacing var * Whoops + button updates * Multile file uploader should return [] when empty * Visual design updates * linting/cleanup * Revert rejectFiles change * Fix tests * Address PR comments * Fix test * addressing quick pr comments * Whoops * fix tests * Readable tests * setState with function * [File Uploader] Update deprecation (#2123) * Update deprecation * Update test for deprecation * Fix docstrings (#2121) * [File Uploader] Missed visual tweaks (#2124) * Quick visual tweaks * linting * [File uploader] Single upload reruns only once (#2122) * Single file uploader replaces in 1 request * Update comments
3 participants
Completely redesigned
st.file_uploaderwith the following additional features. The redesigned file uploader also closes out the following bugs. This redesign does not include file persistence on disk or file picker functionalities. (Closes #1202, Closes #831)Features
Bugs
Demos
This is the initial set of changes. There are open items needed prior to release
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.