Skip to content

Conversation

@louwie17
Copy link
Contributor

@louwie17 louwie17 commented Jul 27, 2022

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:

  1. Load this PR and start the onboarding wizard on a fresh WooCommerce site
  2. Step through the onboarding wizard and make sure the Store details and Business detail forms work correctly still
  3. Check that the tests work correctly
  4. You can create your own component as per the README and see if things work as expected.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added the package: @woocommerce/components issues related to @woocommerce/components label Jul 27, 2022
@louwie17 louwie17 marked this pull request as ready for review July 27, 2022 18:05
@louwie17 louwie17 requested a review from a team July 27, 2022 18:05
@github-actions
Copy link
Contributor

github-actions bot commented Jul 27, 2022

Test Results Summary

Commit SHA: 75235fb

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests87280201171m 7s
E2E Tests185001018616m 25s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Copy link
Contributor

@mattsherman mattsherman left a 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 > >(
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 },
Copy link
Contributor

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 >?

Copy link
Contributor Author

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.

@louwie17 louwie17 force-pushed the add/32_react_provider_to_form_component branch from ecf174e to 75235fb Compare August 3, 2022 19:20
@octaedro octaedro self-requested a review August 4, 2022 13:17
Copy link
Contributor

@octaedro octaedro left a 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 :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: @woocommerce/components issues related to @woocommerce/components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants