-
Notifications
You must be signed in to change notification settings - Fork 43
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
✨ Add upload YAML questionnaire form #1290
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #1290 +/- ##
=======================================
Coverage 43.04% 43.04%
=======================================
Files 145 145
Lines 4330 4330
Branches 999 999
=======================================
Hits 1864 1864
Misses 2454 2454
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9469b44
to
9863525
Compare
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.
Hi there :)
This looks really great!
Added some suggestions
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
<Form onSubmit={handleSubmit(onSubmit)}> | ||
<HookFormPFGroupController | ||
control={control} | ||
name="yamlFile" |
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 see many "yamlFile" occurences here, maybe extract it to a const?
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.
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 meant maybe to add at the top of the file adding something like: const yamlID = "yamlFile"
or similar const name so if the type changes in the future, you can change it in only one place and not multiple places. If it's not possible because of the type inferring than please disregard this.
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.
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.
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.
gotcha, thanks for the explanation 😄
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Show resolved
Hide resolved
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
8d98398
to
8176e70
Compare
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.
Looking real good here! 💯
<Form onSubmit={handleSubmit(onSubmit)}> | ||
<HookFormPFGroupController | ||
control={control} | ||
name="yamlFile" |
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 meant maybe to add at the top of the file adding something like: const yamlID = "yamlFile"
or similar const name so if the type changes in the future, you can change it in only one place and not multiple places. If it's not possible because of the type inferring than please disregard this.
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.
Message text typo and async handler suggestion. Only the typo is a blocker.
onFileInputChange={async (_, file) => { | ||
console.log("uploading file", file); | ||
//TODO: handle new api here. This is just a placeholder. | ||
handleFileUpload(file) | ||
.then((res) => { | ||
setFocus(name); | ||
clearErrors(name); | ||
trigger(name); | ||
}) | ||
.catch((err) => { | ||
//Handle new api error 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.
With the async handler, probably better to use try/await/catch structure:
onFileInputChange={async (_, file) => {
console.log("uploading file", file);
//TODO: handle new api here. This is just a placeholder.
try {
await handleFileUpload(file);
setFocus(name);
clearErrors(name);
trigger(name);
} catch (err) {
//Handle new api error here
}
}}
8176e70
to
e6fb7ba
Compare
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.
+1 to @sjd78's try/await/catch suggestion, but agreed that it's not a blocker. Otherwise LGTM
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.
Just the try/await/catch handler, but looks good otherwise.
client/src/app/pages/assessment/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
e6fb7ba
to
f4eabca
Compare
Signed-off-by: ibolton336 <ibolton@redhat.com>
f4eabca
to
054c0e5
Compare
Closes Import questionnaires