Closed kanta1207 closed 7 months ago
@kanta1207 Thank you for your review! It looks good to go :)
There is only one comment from me to share my concern: For the
FormUsageExample.tsx
, I purposely wrapped the entire<DrawerComponent>...</DrawerComponent>
with the<Form>...</Form>
tags because otherwise, some error occurred that was related to the Radix UI's drawer component. I don't remember what the error looked like because it was a bit long ago, but I don't see the error anymore, even if I restructure the example file. So, I thought your implementation in this PR is completely fine for now, although it might be better to stick to a team-defined specific structure, though.
@uskayyyyy Thank you for your review!
As I wondered in Review points ⬇️ ,
I separated Form logic from CreateGroupButton component, which made me pass isOpen and onClose props to CreateGroupContentDrawer. If it seems like unnecessary props to you, we can also consider putting everything in CreateGroupButton .
My intention was only to separate the Form
logic from CreateGroupButton
, because putting everything in CreateGroupButton
seemed to be too big responsibility in one component.
However, I'm not 100% sure about separating it, and if you don't feel it's too much responsibility to put Form
logic in CreateGroupButton
, I'd like to follow how it's implemented in FormUsageExample.tsx
. , and put Form
logic in CreateGroupButton.tsx
.
Please share your opinion once again 🙏
@kanta1207
However, I'm not 100% sure about separating it, and if you don't feel it's too much responsibility to put Form logic in CreateGroupButton, I'd like to follow how it's implemented in FormUsageExample.tsx. , and put Form logic in CreateGroupButton.tsx.
I understand your concern. What I meant in my earlier comment
..., although it might be better to stick to a team-defined specific structure, though.
is that it would be better to decide which structure we choose, either of the following two cases. Whichever we choose, the <Form>
tags will be within the CreateGroupDrawerContent
component.
Wrap only the necessary parts with the<Form>
. In most cases, the <Form>
tags should wrap <DrawerBody>
and <DrawerFooter>
.
<DrawerContent>
<DrawerHeader>...</DrawerHeader>
<Form {...form}>
<form onSubmit={form.handleSubmit(processSubmit)}>
<DrawerBody>...</DrawerBody>
<DrawerFooter>...</DrawerFooter>
</form>
</Form>
</DrawerContent>
Always wrap the entire <DrawerContent>
with <Form>
even if the DrawerHeader
does not contain form-related elements.
This looks better:
<Form {...form}>
<DrawerContent>
<form onSubmit={form.handleSubmit(processSubmit)}>
<DrawerHeader>...</DrawerHeader>
<DrawerBody>...</DrawerBody>
<DrawerFooter>...</DrawerFooter>
</form>
</DrawerContent>
</Form>
Or, (I forgot why I chose this structure for FormUsageExample
)
<Form {...form}>
<form onSubmit={form.handleSubmit(processSubmit)}>
<DrawerContent>
<DrawerHeader>...</DrawerHeader>
<DrawerBody>...</DrawerBody>
<DrawerFooter>...</DrawerFooter>
</DrawerContent>
</form>
</Form>
I just wanted to share my thoughts. This PR is good enough for now! Let's discuss more deeply when the issue comes up again.
Thank you for sharing your thoughts! I think the Form in this PR is following option 1, and at least it makes sense to me!
For now, I'll merge this PR and as you say, let's discuss again when it's needed!
Overview
Form
component inCreateGroupDrawerContent
.Changes
form
tag with commonForm
component.Review points
Form
logic with react-hook-form is properly implementedCreateGroupButton
component, which made me passisOpen
andonClose
props toCreateGroupContentDrawer
. If it seems like unnecessary props to you, we can also consider putting everything inCreateGroupButton
.Screen Captures
https://github.com/genesis-tech-tribe/nishiki-frontend/assets/99339182/157a8568-1ea6-4255-9022-2a453321c81f
Assignee Checklist:
Reviewer Checklist: