- Notifications
You must be signed in to change notification settings - Fork1k
chore: add task feedback dialog component#20252
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
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.
Had a few suggestions, but nothing worth blocking over
| // All methods must be defined with arrow function syntax. See the docstring | ||
| // above the ApiMethods class for a full explanation. | ||
| exporttypeTaskFeedbackRate="good"|"regular"|"bad"; |
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.
Nit: I feel like this should be "rating" instead of "rate"
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 also don't know if "regular" sounds like natural English. I would expect something like "okay" or "satisfactory"
| import{useMutation}from"react-query"; | ||
| typeTaskFeedbackFormValues={ | ||
| rate:TaskFeedbackRate|null; |
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.
Same feedback here: I feel like this should also be "rating"
| @@ -0,0 +1,145 @@ | |||
| importtype{DialogProps}from"@radix-ui/react-dialog"; | |||
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 want theDialogProps from Radix, or do you think we could use the version in theDialog file? It feels to me that we're importing so much from that file, but then importing one extra type definition from a different "layer"
| onSubmit:(values)=>{ | ||
| createFeedback(valuesasCreateTaskFeedbackRequest); | ||
| }, |
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 haven't really used Formik yet, so I'm not completely sure: does it provide any safety nets to make sure that we don't submit when the rating is null? Because if not, I feel like the code should be something like this:
if(values.rate!==null){createFeedback({rate:values.rate,comment:values.comment,});}
It creates an extra intermediary object, but I'd rather have that than a type assertion that could potentially lead to junk data getting sent to the server
| </RateOption> | ||
| <RateOption{...formik.getFieldProps("rate")}value="regular"> | ||
| <MehIcon/> | ||
| It sort-of worked, but struggled a lot |
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.
Nit: I think the hyphen should be removed
| <div | ||
| className="flex flex-col gap-1" | ||
| role="radiogroup" | ||
| aria-label="Rate your experience" | ||
| > |
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'm not fully sure if we need thediv. I feel like we could use thefieldset andlegend elements instead
| exportconstSubmitting:Story={ | ||
| beforeEach:async()=>{ | ||
| spyOn(API.experimental,"createTaskFeedback").mockImplementation(()=>{ | ||
| returnnewPromise(()=>{}); |
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 want to return a promise like this? Right now it can't ever resolve
| constregularOption=body.getByLabelText( | ||
| "It sort-of worked, but struggled a lot", | ||
| ); | ||
| userEvent.click(regularOption); | ||
| constcommentTextarea=body.getByLabelText("Additional comments"); | ||
| awaituserEvent.type(commentTextarea,"This is my comment"); |
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 haven't actually run this code, but I'mpretty sure it works. I'd really like to see role selectors for these:
constregularOption=body.getByRole("radio",{name:"It sort-of worked, but struggled a lot",});userEvent.click(regularOption);constcommentTextarea=body.getByRole("textbox",{name:"Additional comments",});awaituserEvent.type(commentTextarea,"This is my comment");
| constregularOption=body.getByLabelText( | ||
| "It sort-of worked, but struggled a lot", | ||
| ); | ||
| userEvent.click(regularOption); | ||
| constcommentTextarea=body.getByLabelText("Additional comments"); | ||
| awaituserEvent.type(commentTextarea,"This is my comment"); |
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'd like to see these updated, too
| constregularOption=body.getByLabelText( | ||
| "It sort-of worked, but struggled a lot", | ||
| ); | ||
| userEvent.click(regularOption); | ||
| constcommentTextarea=body.getByLabelText("Additional comments"); | ||
| awaituserEvent.type(commentTextarea,"This is my comment"); |
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.
And also these
Related to#20214
This PR aims to add only the FE component for capturing user feedback from tasks. Once the BE work is completed (in a separate PR), this component will be triggered after a task is deleted.
The goal is to develop this feature in parallel.
Screenshot:
