feat(EMI-2196): Consolidated global AddressFormFields component + usage in Auction/Invoice apps #14898

erikdstock · 2024-11-22T18:56:26Z

The type of this PR is: feat

The changes are relatively small when viewed commit-by-commit, at least for the early ones.

Description

This PR relates to EMI-2196 and completes EMI-2205. It consolidates our Auction + Invoice address forms to use a single <AddressFormFields /> layout. This component includes its own validation schema and the utils files in the src/Components/Address has new helpers for working with and testing that. There are new tests to replace Because the phone number input is sometimes not required, it can be controlled with a prop.

Because we consolidate forms, we also get US address autocomplete in the invoice form for free (this could also be controlled with a prop if we wanted).

cc @artsy/emerald-devs

Invoice app, before and after.

Invoice form with autocomplete

Followup work

This opens the door to a lot of cleanup in our checkout flow, which includes several redundant types.
- The checkout/shipping modal should be easy to migrate to this. The full-page version will be a little harder because of the extra instrumentation involved (for example in hiding shipping quotes)
- We can also move off of the credit card payment form there which is extra complicated because it does not use formik.
- The address form filling helper fillAddressFormFields() created for this specific layout might be more efficient than the multiple similar helpers called fillAddressForm(). This migration could therefore help address EMI-1685.
We can update the user settings page to use this layout as well, but first we'll want to integrate the rich phone input into this layout along with the rich phone validation.

cc @artsy/emerald-devs

- maybe move phone number into address object on address type since it is already redundantly in there?

relativeci · 2024-11-22T18:59:13Z

#1017 Bundle Size — 9.54MiB (-0.07%).

13be370(current) vs ffe4f84 main#484(baseline)

Important

Bundle introduced 1 and removed 4 duplicate packages – View changed duplicate packages

Warning

Bundle introduced 3 new packages: web-vitals, @sentry-internal/browser-utils, stylis – View changed packages

Bundle metrics

9 changes

1 regression

3 improvements

	Current #1017	Baseline #484
Initial JS	`3.69MiB`(`-6.58%`)	`3.95MiB`
Initial CSS	`0B`	`0B`
Cache Invalidation	`87.74%`	`2.04%`
Chunks	`141`(`-1.4%`)	`143`
Assets	`145`(`-0.68%`)	`146`
Modules	`5690`(`+0.94%`)	`5637`
Duplicate Modules	`495`(`+8.79%`)	`455`
Duplicate Code	`5.87%`(`-0.17%`)	`5.88%`
Packages	`282`(`-3.09%`)	`291`
Duplicate Packages	`39`(`-7.14%`)	`42`

Bundle size by type

2 changes

1 regression

1 improvement

	Current #1017	Baseline #484
JS	`9.33MiB` (`+0.2%`)	`9.31MiB`
Other	`212.46KiB` (`-10.67%`)	`237.84KiB`

Bundle analysis report Branch erik.emi2196-invoice-address-for... Project dashboard

^{Generated by RelativeCI Documentation Report issue}

erikdstock · 2024-11-22T18:58:22Z

src/Apps/Auction/Components/Form/AddressFormWithCreditCard.tsx

@@ -10,7 +11,7 @@ export const AddressFormWithCreditCard: React.FC<React.PropsWithChildren<unknown
    setFieldError,
    errors,
    touched,
-  } = useFormContext()
+  } = useAuctionFormContext()


this changed because the name was coming up in searches in multiple places and caused me to confuse myself. Maybe not worth it though.

erikdstock · 2024-11-22T18:59:39Z

src/Components/Address/AddressFormFields.tsx

+  phoneNumber: string
+}
+
+export const AddressFormFields = <V extends FormikContextWithAddress>() => {


Type parameter here helps guarantee that the useFormikContext() call below will be able to service our fields.

erikdstock · 2024-11-22T20:36:10Z

I'll re-open this when it's ready for merge.

erikdstock · 2024-11-23T05:15:16Z

src/Apps/Auction/Hooks/useCreateTokenAndSubmit.ts

+    if (!values.address) {
+      helpers.setStatus("SUBMISSION_FAILED")
+      return
+    }


This caused a bunch of test failures [fixed here] even though I don't think it would be an issue in real life due to form validations. I only did it to satisfy some ts complaints about using the ! so we could remove it.

erikdstock · 2024-11-23T05:25:14Z

Hmm, I see some more validation schemas that I might need to reconcile:

Auction app validation schemas
AuctionBidRoute uses some other schemas from the previous file

OTOH maybe this is ok as long as the validations work - but it would be nice to keep it all consistent and in one place.

erikdstock

PR tour.

erikdstock · 2024-11-25T15:43:57Z

src/Components/Address/AddressForm.tsx

-export interface Address {
-  name: string
-  country: string
-  postalCode: string
-  addressLine1: string
-  addressLine2: string
-  city: string
-  region: string
-  phoneNumber?: string
-}


PR Tour start: This file contains a non-formik address form layout which is only used on the checkout>payment route. I'm considering it deprecated so moved most utility types and constants to a nearby file.

erikdstock · 2024-11-25T15:47:59Z

src/Components/Address/AddressFormFields.tsx

+interface FormikContextWithAddress {
+  address: Address
+  phoneNumber?: string
+}


Most of the PR is here: I promoted the Apps/Auction/Components/Form/AddressForm.tsx file to live in Components/Address/AddressFormFields.tsx. It is now used by both the auction and invoice apps for their credit card forms, and is named with Fields because it is only the fields.

This file also exports a non-Component helper for generating the yup form validations. Both the helper and the component have a prop/arg for whether to include the phone number.

The type argument of the AddressFormFields component helps guarantee that it will be used in within a formik context that matches those fields

erikdstock · 2024-11-25T15:49:09Z

src/Components/Address/AddressAutocompleteInput.tsx

@@ -312,6 +312,7 @@ export const AddressAutocompleteInput = ({
        onChange={onChange}
        error={error}
        data-testid={dataTestId}
+        required={!!autocompleteProps.required}


The address autocomplete input is incorrectly not passing the required prop to its fallback input - likely a bug in the existing shipping route where this input is used, but not likely one many people hit since they generally have an address.

erikdstock · 2024-11-25T15:50:43Z

src/Components/Address/__tests__/AddressFormFields.jest.tsx

+describe("AddressForm", () => {
+  const mockOnSubmit = jest.fn()
+  beforeEach(() => {
+    mockOnSubmit.mockClear()
+  })
+
+  describe("without phone number input", () => {


A port of the Auction AddressForm.jest.enzyme.tsx file, now with more assertions.

erikdstock · 2024-11-25T15:52:13Z

src/Components/Address/__tests__/utils.ts

+export const fillAddressFormFields = async (address: Partial<Address>) => {
+  const { country, phoneNumber, ...defaultTextInputs } = address


I'm hopeful this can replace our multiple other fillAddressForm() helpers and do so in a fast way since we have several test suites completely disable in CI due to timeouts specifically tied to these helpers (see EMI-1685).

erikdstock · 2024-11-25T15:54:08Z

src/Components/Address/utils.ts

+export const yupAddressValidator = Yup.object().shape({
+  name: Yup.string().required("Full name is required"),
+  addressLine1: Yup.string().required("Street address is required"),
+  addressLine2: Yup.string().nullable(),
+  city: Yup.string().required("City is required"),
+  postalCode: postalCodeValidator,
+  region: Yup.string().when("country", {
+    is: country => ["US", "CA"].includes(country),
+    then: Yup.string().required("State is required"),
+    otherwise: Yup.string(),
+  }),
+  country: Yup.string().required("Country is required"),
+})


Copied from our refactored checkout shipping route validations- between this and the user settings>shipping address validations (worth a comparison maybe) I think we can get a single validation schema.

erikdstock · 2024-11-25T15:56:18Z

src/Apps/Invoice/Components/InvoicePaymentForm.tsx

@@ -28,6 +32,9 @@ export const InvoicePaymentForm: React.FC<React.PropsWithChildren<InvoicePayment
    <Formik<AddressFormValues>
      onSubmit={handleSubmit}
      initialValues={{ address: emptyAddress, creditCard: false }}
+      validationSchema={Yup.object().shape({
+        ...addressFormFieldsValidator({ withPhoneNumber: false }),


using our address form fields validator in a yup schema. strangely this formik context didn't seem to have any validation... maybe because it relied on stripe and we don't actually store any address?

erikdstock · 2024-11-25T15:59:19Z

src/Apps/Invoice/Components/AddressFormWithCreditCard.tsx

@@ -36,7 +43,7 @@ export const AddressFormWithCreditCard: React.FC<React.PropsWithChildren<unknown
        required
      />

-      <AddressForm />
+      <AddressFormFields<AddressFormValues> />


Using our new AddressFormFields component with the formik type param to mostly guarantee that our context will satisfy those fields*

I say mostly because if you aren't including a phone number then you shouldn't pass the withPhoneNumber prop. I think we could get fancier with our function component type to give it multiple signatures (if your type doesn't have a phone number, then your withPhoneNumber prop can't be true. But then you get into the validation schema constructor function which is its own separate thing in a different file and I thought this is too much magic. Maybe a happy medium would be better doc comments in the file.

erikdstock · 2024-11-25T15:59:52Z

src/Apps/Auction/Components/Form/AddressFormWithCreditCard.tsx

@@ -44,7 +47,7 @@ export const AddressFormWithCreditCard: React.FC<React.PropsWithChildren<unknown
        required
      />

-      <AddressForm />
+      <AddressFormFields<AuctionFormValues> withPhoneNumber />


And here as well is the auction registration route using our new form withPhoneNumber. I wondered if i should use the schema constructor helper mentioned earlier in this tour on the formik context here but haven't done it yet.

damassi · 2024-11-25T17:00:12Z

src/Components/Address/AddressFormFields.tsx

+}
+
+export const addressFormFieldsValidator = (args: {
+  withPhoneNumber: boolean


Should this be typed with Props right up above?

damassi

PR looks great @erikdstock 💯

Only feedback is: The PR title seems a bit misleading considering everything that's going on in here. If we can perhaps summarize it there (for search) that'd be ideal, but otherwise 🍏

erikdstock added 5 commits November 22, 2024 12:51

promote address form fields to generic component

3fa984d

fix non-null assertions

fdf0fca

migrate auction registration form to use global address component

402b75a

types cleanup to guarantee form used in correct ctx

307fabe

- maybe move phone number into address object on address type since it is already redundantly in there?

port old enzyme test to new form

4f3c9cc

erikdstock requested review from damassi and mzikherman November 22, 2024 18:56

artsy-peril bot added the Jira Synced label Nov 22, 2024

erikdstock self-assigned this Nov 22, 2024

erikdstock commented Nov 22, 2024

View reviewed changes

erikdstock closed this Nov 22, 2024

damassi removed request for damassi and mzikherman November 22, 2024 20:38

erikdstock added 4 commits November 22, 2024 22:01

add more tests

74c79a5

implement no-phone invoice address form

d9ee50c

revert changes to shipping route types for now

b4a2c84

cleanup

5e26bfb

erikdstock reopened this Nov 23, 2024

erikdstock requested a review from starsirius November 23, 2024 04:33

fix tests

defdffe

erikdstock requested a review from rquartararo November 23, 2024 05:12

erikdstock commented Nov 23, 2024

View reviewed changes

erikdstock marked this pull request as ready for review November 23, 2024 05:15

delete unused AddressForm

13be370

erikdstock commented Nov 25, 2024

View reviewed changes

damassi reviewed Nov 25, 2024

View reviewed changes

damassi approved these changes Nov 25, 2024

View reviewed changes

erikdstock changed the title ~~feat(EMI-2196): Invoice address form layout~~ feat(EMI-2196): Consolidated global AddressFormFields component + usage in Auction/Invoice apps Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(EMI-2196): Consolidated global AddressFormFields component + usage in Auction/Invoice apps #14898

feat(EMI-2196): Consolidated global AddressFormFields component + usage in Auction/Invoice apps #14898

erikdstock commented Nov 22, 2024 •

edited by jira bot

Loading

relativeci bot commented Nov 22, 2024 •

edited

Loading

erikdstock Nov 22, 2024

erikdstock Nov 22, 2024

erikdstock commented Nov 22, 2024

erikdstock Nov 23, 2024 •

edited

Loading

erikdstock commented Nov 23, 2024

erikdstock left a comment

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

erikdstock Nov 25, 2024

damassi Nov 25, 2024

damassi left a comment

		export const fillAddressFormFields = async (address: Partial<Address>) => {
		const { country, phoneNumber, ...defaultTextInputs } = address

feat(EMI-2196): Consolidated global AddressFormFields component + usage in Auction/Invoice apps #14898

Are you sure you want to change the base?

feat(EMI-2196): Consolidated global AddressFormFields component + usage in Auction/Invoice apps #14898

Conversation

erikdstock commented Nov 22, 2024 • edited by jira bot Loading

Description

Followup work

relativeci bot commented Nov 22, 2024 • edited Loading

#1017 Bundle Size — 9.54MiB (-0.07%).

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdstock commented Nov 22, 2024

erikdstock Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

erikdstock commented Nov 23, 2024

erikdstock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damassi left a comment

Choose a reason for hiding this comment

erikdstock commented Nov 22, 2024 •

edited by jira bot

Loading

relativeci bot commented Nov 22, 2024 •

edited

Loading

erikdstock Nov 23, 2024 •

edited

Loading