-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FS-93 Changes for the File Upload UI #36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 💯 I just got a couple small comments
} | ||
|
||
.uploadButton:active { | ||
background-color: var(--grey-400); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed when clicking the button the background has a square shape. I think that can be an easy fix by adding border-radius: 50%;
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
.uploadedFile { | ||
font-size: 14px; | ||
color: var(--grey-900); | ||
background-color: var(--grey-100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the background colour here should be --grey-200
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
overflow: hidden; | ||
text-overflow: ellipsis; | ||
margin-right: 65px; | ||
border: 1px solid var(--grey-300); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a nit-pick but I don't think we should have a border on this card here as per the UX specs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
margin-bottom: 4px; | ||
box-sizing: border-box; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed some extra spacing between the inputContainer and the uploadedFileContainer which I think can be easily fixed by removing the margin from the inputContainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
frontend/src/components/input.tsx
Outdated
@@ -8,7 +8,8 @@ import React, { | |||
} from 'react'; | |||
import styles from './input.module.css'; | |||
import RightArrowIcon from '../icons/send.svg'; | |||
import UploadIcon from '../icons/upload.svg'; | |||
import { FileUploader } from './fileUploader'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call it FileUpload
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
isUploading: boolean; | ||
isUploaded: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isUploading: boolean; | |
isUploaded: boolean; | |
uploadInProgress: boolean; | |
uploadComplete: boolean; |
I would suggest these names are slightly nicer to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
frontend/src/components/input.tsx
Outdated
const [isUploading, setIsUploading] = useState<boolean>(false); | ||
const [isUploaded, setIsUploaded] = useState<boolean>(false); | ||
// TODO: Add fileId to the state | ||
// const [fileId, setFileId] = useState<string>(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for future use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if we needed fileId, but there are other ways and depends upon the design to get that report, so I have removed it.
frontend/src/components/input.tsx
Outdated
const formData = new FormData(); | ||
formData.append('file', file); | ||
const response = await fetch(`${process.env.BACKEND_URL}/uploadfile`, { | ||
method: 'POST', | ||
body: formData, | ||
credentials: 'include', | ||
}); | ||
if (!response.ok) | ||
throw new Error(`Upload failed with status ${response.status}`); | ||
|
||
const { filename, id } = await response.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this request and response bit to server.ts
to match the other api calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
frontend/src/components/input.tsx
Outdated
// setFileId(id); | ||
setUploadedFile(file); | ||
setIsUploading(false); | ||
setIsUploaded(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the isUploaded
state? We could maybe just use the fact the uploadedFile
is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Initially I used it for quick work, but then forgot to use the uploadedFile and remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Haven't pulled it down since the changes and tested myself though
|
||
const tooltipContent = disabled ? ( | ||
<> | ||
<p>You already uploaded one file.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches design, but should probably be You've
instead of You
.
Description
These changes are to do the UI changes for File Upload, backend is already done.
Changelog