-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add/32 react provider to form component #34082
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
Conversation
Test Results SummaryCommit SHA: 75235fb
To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
mattsherman
left a 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 took a look at the code. I do not consider myself familiar enough with TypeScript to formally review this code. I left a few questions, which may be actual issues, or just due to my lack of knowledge.
| /** | ||
| * A form component to handle form state and provide input helper props. | ||
| */ | ||
| function Form< Values extends Record< string, string > >( |
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 that familiar with TypeScript, but it feels odd to define Values here when it is used above in the file. Seems confusing.
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.
Can't form field values be of types other than string?
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 end up removing string here with any and used any instead of unknown is several different other places, as that really makes the most sense with this Form component.
In relation to your first point, the Values defined above is consolidated to the FormProps type.
Given FormProps takes a generic that means we should also pass a generic into the Form component that will in turn get passed to FormProps -> props: PropsWithChildren< FormProps< Values > >
This would allow us to create a Form component as such: <Form<Product>></Form> meaning that Values is automatically replaced by the Product type and so the initialValues prop will only accept values in the format of Product nothing else.
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.
Btw I make use of extends here, which is TypeScripts way of saying: you can only pass a generic type that follows the Record< string, string > format.
Meaning that I can't do something like this: <Form<string>></Form>, but <Form<{ name: string }></Form> is valid.
| * Function to call when a value changes in the form. | ||
| */ | ||
| onChange?: ( | ||
| value: { name: string; value: unknown }, |
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.
Why is it value: unknown when Values is Record< string, string >?
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.
Good point actually, I might of missed this ( to many occurrences of close to the same types) I replace the unknown with any and replaced Record< string, string > with Record< string, any > to more accurately reflect a Form object.
ecf174e to
75235fb
Compare
octaedro
left a 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.
Great job @louwie17! I'm not a TypeScript expert but as far as I could see the code looks good and is testing well here ![]()
All Submissions:
Changes proposed in this Pull Request:
This adds the use of a React context provider to the Form component. This will allow nested fields to more easily make use of the Form data without having to property drill down.
Closes https://github.com/woocommerce/mothra-private/issues/32
How to test the changes in this Pull Request:
Other information:
pnpm changelog add --filter=<project>?FOR PR REVIEWER ONLY: