- 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?
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2669,6 +2669,13 @@ class ApiMethods { | ||
| // | ||
| // 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 commentThe 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 commentThe 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" | ||
| exporttypeCreateTaskFeedbackRequest={ | ||
| rate:TaskFeedbackRate; | ||
| comment?:string; | ||
| }; | ||
| classExperimentalApiMethods{ | ||
| constructor(protectedreadonlyaxios:AxiosInstance){} | ||
| @@ -2732,6 +2739,15 @@ class ExperimentalApiMethods { | ||
| deleteTask=async(user:string,id:string):Promise<void>=>{ | ||
| awaitthis.axios.delete(`/api/experimental/tasks/${user}/${id}`); | ||
| }; | ||
| createTaskFeedback=async( | ||
| _taskId:string, | ||
| _req:CreateTaskFeedbackRequest, | ||
| )=>{ | ||
| returnnewPromise<void>((res)=>{ | ||
| setTimeout(()=>res(),500); | ||
| }); | ||
| }; | ||
| } | ||
| // This is a hard coded CSRF token/cookie pair for local development. In prod, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| import{MockTask,mockApiError}from"testHelpers/entities"; | ||
| import{withGlobalSnackbar}from"testHelpers/storybook"; | ||
| importtype{Meta,StoryObj}from"@storybook/react-vite"; | ||
| import{API}from"api/api"; | ||
| import{expect,spyOn,userEvent,within}from"storybook/test"; | ||
| import{TaskFeedbackDialog}from"./TaskFeedbackDialog"; | ||
| constmeta:Meta<typeofTaskFeedbackDialog>={ | ||
| title:"modules/tasks/TaskFeedbackDialog", | ||
| component:TaskFeedbackDialog, | ||
| args:{ | ||
| taskId:MockTask.id, | ||
| open:true, | ||
| }, | ||
| }; | ||
| exportdefaultmeta; | ||
| typeStory=StoryObj<typeofTaskFeedbackDialog>; | ||
| exportconstIdle:Story={}; | ||
| exportconstSubmitting:Story={ | ||
| beforeEach:async()=>{ | ||
| spyOn(API.experimental,"createTaskFeedback").mockImplementation(()=>{ | ||
| returnnewPromise(()=>{}); | ||
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. Do we want to return a promise like this? Right now it can't ever resolve | ||
| }); | ||
| }, | ||
| play:async({ canvasElement, step})=>{ | ||
| constbody=within(canvasElement.ownerDocument.body); | ||
| step("fill and submit the form",async()=>{ | ||
| 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"); | ||
Comment on lines +32 to +38 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. 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"); | ||
| constsubmitButton=body.getByRole("button",{ | ||
| name:"Submit Feedback", | ||
| }); | ||
| awaituserEvent.click(submitButton); | ||
| }); | ||
| }, | ||
| }; | ||
| exportconstSuccess:Story={ | ||
| args:{ | ||
| open:true, | ||
| }, | ||
| decorators:[withGlobalSnackbar], | ||
| beforeEach:async()=>{ | ||
| spyOn(API.experimental,"createTaskFeedback").mockResolvedValue(); | ||
| }, | ||
| play:async({ canvasElement, step})=>{ | ||
| constbody=within(canvasElement.ownerDocument.body); | ||
| step("fill and submit the form",async()=>{ | ||
| 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"); | ||
Comment on lines +60 to +66 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. I'd like to see these updated, too | ||
| constsubmitButton=body.getByRole("button",{ | ||
| name:"Submit Feedback", | ||
| }); | ||
| awaituserEvent.click(submitButton); | ||
| }); | ||
| step("submitted successfully",async()=>{ | ||
| awaitbody.findByText("Feedback submitted successfully"); | ||
| expect(API.experimental.createTaskFeedback).toHaveBeenCalledWith( | ||
| MockTask.id, | ||
| { | ||
| rate:"regular", | ||
| comment:"This is my comment", | ||
| }, | ||
| ); | ||
| }); | ||
| }, | ||
| }; | ||
| exportconstFailure:Story={ | ||
| beforeEach:async()=>{ | ||
| spyOn(API.experimental,"createTaskFeedback").mockRejectedValue( | ||
| mockApiError({ | ||
| message:"Failed to submit feedback", | ||
| detail:"Server is down", | ||
| }), | ||
| ); | ||
| }, | ||
| play:async({ canvasElement, step})=>{ | ||
| constbody=within(canvasElement.ownerDocument.body); | ||
| step("fill and submit the form",async()=>{ | ||
| 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"); | ||
Comment on lines +100 to +106 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. And also these | ||
| constsubmitButton=body.getByRole("button",{ | ||
| name:"Submit Feedback", | ||
| }); | ||
| awaituserEvent.click(submitButton); | ||
| }); | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| importtype{DialogProps}from"@radix-ui/react-dialog"; | ||
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. Do we want the | ||
| import{ | ||
| API, | ||
| typeCreateTaskFeedbackRequest, | ||
| typeTaskFeedbackRate, | ||
| }from"api/api"; | ||
| import{ErrorAlert}from"components/Alert/ErrorAlert"; | ||
| import{Button}from"components/Button/Button"; | ||
| import{ | ||
| Dialog, | ||
| DialogClose, | ||
| DialogContent, | ||
| DialogDescription, | ||
| DialogFooter, | ||
| DialogHeader, | ||
| DialogTitle, | ||
| }from"components/Dialog/Dialog"; | ||
| import{displaySuccess}from"components/GlobalSnackbar/utils"; | ||
| import{Spinner}from"components/Spinner/Spinner"; | ||
| import{Textarea}from"components/Textarea/Textarea"; | ||
| import{useFormik}from"formik"; | ||
| import{FrownIcon,MehIcon,SmileIcon}from"lucide-react"; | ||
| importtype{FC,HTMLProps,ReactNode}from"react"; | ||
| import{useMutation}from"react-query"; | ||
| typeTaskFeedbackFormValues={ | ||
| rate:TaskFeedbackRate|null; | ||
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. Same feedback here: I feel like this should also be "rating" | ||
| comment:string; | ||
| }; | ||
| typeTaskFeedbackDialogProps=DialogProps&{ | ||
| taskId:string; | ||
| }; | ||
| exportconstTaskFeedbackDialog:FC<TaskFeedbackDialogProps>=({ | ||
| taskId, | ||
| ...dialogProps | ||
| })=>{ | ||
| const{ | ||
| mutate:createFeedback, | ||
| error, | ||
| isPending, | ||
| }=useMutation({ | ||
| mutationFn:(req:CreateTaskFeedbackRequest)=> | ||
| API.experimental.createTaskFeedback(taskId,req), | ||
| onSuccess:()=>{ | ||
| displaySuccess("Feedback submitted successfully"); | ||
| }, | ||
| }); | ||
| constformik=useFormik<TaskFeedbackFormValues>({ | ||
| initialValues:{ | ||
| rate:null, | ||
| comment:"", | ||
| }, | ||
| onSubmit:(values)=>{ | ||
| createFeedback(valuesasCreateTaskFeedbackRequest); | ||
| }, | ||
Comment on lines +56 to +58 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. 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 | ||
| }); | ||
| constisRateSelected=Boolean(formik.values.rate); | ||
| return( | ||
| <Dialog{...dialogProps}> | ||
| <DialogContent> | ||
| <DialogHeader> | ||
| <DialogTitle>Task feedback</DialogTitle> | ||
| <DialogDescription> | ||
| Your feedback is important to us. Please rate your experience with | ||
| this task. | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <form | ||
| id="feedback-form" | ||
| onSubmit={formik.handleSubmit} | ||
| className="flex flex-col gap-4" | ||
| > | ||
| {error&&<ErrorAlerterror={error}/>} | ||
| <div | ||
| className="flex flex-col gap-1" | ||
| role="radiogroup" | ||
| aria-label="Rate your experience" | ||
| > | ||
Comment on lines +81 to +85 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. I'm not fully sure if we need the | ||
| <RateOption{...formik.getFieldProps("rate")}value="good"> | ||
| <SmileIcon/>I achieved my goal | ||
| </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 commentThe reason will be displayed to describe this comment to others.Learn more. Nit: I think the hyphen should be removed | ||
| </RateOption> | ||
| <RateOption{...formik.getFieldProps("rate")}value="bad"> | ||
| <FrownIcon/> | ||
| It was a flop | ||
| </RateOption> | ||
| </div> | ||
| <labelclassName="sr-only"htmlFor="comment"> | ||
| Additional comments | ||
| </label> | ||
| <Textarea | ||
| id="comment" | ||
| placeholder="Wanna say something else?..." | ||
| className="h-32 resize-none" | ||
| {...formik.getFieldProps("comment")} | ||
| /> | ||
| </form> | ||
| <DialogFooter> | ||
| <DialogCloseasChild> | ||
| <Buttonvariant="outline">Close</Button> | ||
| </DialogClose> | ||
| <Button | ||
| type="submit" | ||
| form="feedback-form" | ||
| disabled={!isRateSelected||isPending} | ||
| > | ||
| <Spinnerloading={isPending}/> | ||
| Submit Feedback | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| }; | ||
| typeRateOptionProps=HTMLProps<HTMLInputElement>&{ | ||
| children:ReactNode; | ||
| }; | ||
| constRateOption:FC<RateOptionProps>=({ children, ...inputProps})=>{ | ||
| return( | ||
| <label | ||
| className={` | ||
| cursor-pointer border border-border border-solid hover:bg-surface-secondary | ||
| px-4 py-3 rounded text-sm has-[:checked]:bg-surface-quaternary | ||
| flex items-center gap-3 [&_svg]:size-4 | ||
| `} | ||
| > | ||
| <inputclassName="hidden"type="radio"{...inputProps}/> | ||
| {children} | ||
| </label> | ||
| ); | ||
| }; | ||
Uh oh!
There was an error while loading.Please reload this page.