-
Notifications
You must be signed in to change notification settings - Fork 492
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
Update Sample File Upload and upgrade buttons to Shadcn button #9157
Changes from 2 commits
8448989
662f437
cef5438
ec36ef5
a7043f6
a341da1
5403593
9d3de71
bae2649
52100c4
18ad9b5
180fa67
ceba9fb
c972c5b
5489cb6
3c6eb0e
b48a7e3
3801776
6e180b3
4598c0c
28ccbc4
fd0bf27
e86f229
66cf623
48a604f
37cfc0e
76ebf4f
0af4cf0
cd09c74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -69,7 +69,6 @@ const UpdateStatusDialog = (props: Props) => { | |||||
const [contentType, setcontentType] = useState<string>(""); | ||||||
const [uploadPercent, setUploadPercent] = useState(0); | ||||||
const [uploadStarted, setUploadStarted] = useState<boolean>(false); | ||||||
const [uploadDone, setUploadDone] = useState<boolean>(false); | ||||||
|
||||||
const currentStatus = SAMPLE_TEST_STATUS.find( | ||||||
(i) => i.text === sample.status, | ||||||
|
@@ -123,7 +122,6 @@ const UpdateStatusDialog = (props: Props) => { | |||||
(xhr: XMLHttpRequest) => { | ||||||
if (xhr.status >= 200 && xhr.status < 300) { | ||||||
setUploadStarted(false); | ||||||
setUploadDone(true); | ||||||
request(routes.editUpload, { | ||||||
pathParams: { | ||||||
id: data.id, | ||||||
|
@@ -156,13 +154,15 @@ const UpdateStatusDialog = (props: Props) => { | |||||
); | ||||||
return e.target.files[0]; | ||||||
}; | ||||||
const removeFile = () => { | ||||||
setfile(undefined); | ||||||
}; | ||||||
const handleUpload = async () => { | ||||||
const f = file; | ||||||
if (f === undefined) return; | ||||||
const category = "UNSPECIFIED"; | ||||||
const name = f.name; | ||||||
setUploadStarted(true); | ||||||
setUploadDone(false); | ||||||
|
||||||
const { data } = await request(routes.createUpload, { | ||||||
body: { | ||||||
|
@@ -236,10 +236,17 @@ const UpdateStatusDialog = (props: Props) => { | |||||
hidden | ||||||
/> | ||||||
</label> | ||||||
{file && ( | ||||||
<CareIcon | ||||||
icon="l-times" | ||||||
className="text-lg cursor-pointer mt-2 mr-4" | ||||||
onClick={removeFile} | ||||||
/> | ||||||
)} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance accessibility for the clear file button The clear file icon button needs accessibility improvements to help screen reader users understand its purpose. {file && (
<CareIcon
icon="l-times"
className="text-lg cursor-pointer mt-2 mr-4"
onClick={removeFile}
+ role="button"
+ aria-label={t("clear_file")}
+ title={t("clear_file")}
/>
)}
|
||||||
<Submit | ||||||
type="submit" | ||||||
onClick={handleUpload} | ||||||
disabled={uploadDone} | ||||||
disabled={file ? false : true} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify the disabled prop logic The current boolean expression can be simplified. - disabled={file ? false : true}
+ disabled={!file} 📝 Committable suggestion
Suggested change
🧰 Tools🪛 Biome[error] 249-249: Unnecessary use of boolean literals in conditional expression. Simplify your code by directly assigning the result without using a ternary operator. (lint/complexity/noUselessTernary) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nithish1018 Great! Thanks for making the change. If you found this review helpful, would you consider giving us a shout-out on X? |
||||||
> | ||||||
<CareIcon icon="l-cloud-upload" className="text-lg" /> | ||||||
<span>Upload</span> | ||||||
|
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.
💡 Codebase verification
🛠️ Refactor suggestion
Add upload state check to prevent file removal during upload
The verification reveals that
uploadStarted
state is properly managed in the component and is used during file uploads. However, theremoveFile
function currently lacks this check, which could lead to inconsistent state if a file is removed during upload.removeFile
function to prevent file removal during active uploadsThe suggested changes in the review comment are valid and necessary:
🔗 Analysis chain
Verify clear file behavior during upload
Please ensure that the clear file functionality is properly disabled when a file upload is in progress to prevent potential issues.
Consider adding an upload state check:
const removeFile = () => { + if (uploadStarted) return; setfile(undefined); setcontentType(""); };
Also applies to: 239-246
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 65
Script:
Length of output: 1590