Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
171 stars 69 forks source link

Replace `intl-tel-input` component with the WooCommerce PhoneNumberInput component #8581

Open mordeth opened 4 months ago

mordeth commented 4 months ago

Description

A new lightweight https://github.com/woocommerce/woocommerce/pull/40335 has been introduced into the WooCommerce core to replace intl-tel-input in WooPayments. This change significantly reduces the bundle size and provides a React-based customizable component.

However, we are still using the intl-tel-input component in two areas within WooPayments that require replacement with the new component.

The two instances where the old component exists are:

Acceptance criteria

anu-rock commented 4 months ago

@mordeth What are the two instances where we are still using the old component?

mordeth commented 4 months ago

The two instances where the old component exists are:

I have described them in the issue description.

RadoslavGeorgiev commented 4 months ago

Note: I'm assigning the "checkout payments" focus label because it would be either this, or in-person payments (which we don't have a label for). The bundle size effects on checkout could be more significant, which is also why I preferred the label.

danielmx-dev commented 1 month ago

Please add your planning poker estimate with Zenhub @FangedParakeet

danielmx-dev commented 1 month ago

After spending a couple days working on this I run into the following issue: The @woocommerce/components version that we are currently using (12.0.0) does not include the PhoneNumberInput component. In order to do so we need to upgrade the package version at least to 12.2.0 but I found the following issues (See a WIP in this draft PR https://github.com/Automattic/woocommerce-payments/pull/9099):

I think we have two options here:

  1. Move forward with the proposed changes in the draft PR https://github.com/Automattic/woocommerce-payments/pull/9099, and then handle the node 20 upgrade in the future to restore the engine-strict=true setting.
  2. Perform the upgrade to node 20 first (in #8129 or create a new issue solely for the dependency upgrades and fixes), then come back and rebase the draft PR and move forward with this issue until that point.
danielmx-dev commented 1 month ago

Now that the node 20 upgrade has been completed I ran into another issue. It looks like @woocommerce/components is NOT registered for the checkout page, only for admin pages. This means that references to that module won't work unless the script is loaded in the admin.

This can be confirmed by going to the blocks checkout and executing window.wc.components in the Developer Tools Console, which will result in undefined, while in an admin page, like Payments -> Settings, the same statement returns a Module {} instance.

It also seems that, at this moment, WooCommerce Core doesn't expose any functions to register the wc-components dependency on our own, and also there's risks associated with including that script in the shop pages. We may also end up increasing the code being downloaded to the browser significantly only to make use of a single component.

In this draft PR https://github.com/Automattic/woocommerce-payments/pull/9155 I tried a workaround which consisted in forcing the inclusion of PhoneNumberInput in the bundle, as well as manually copying the corresponding styles for that component so they are also included in the build. This way the component will work, and although the gains are not as significant as if we used the external dependency, there's still an improvement compared to intl-tel-input (attaching the screenshot since the comment gets edited with every push).

image

danielmx-dev commented 3 weeks ago

One more issue that is definitely a blocker. In the settings, we store account_business_support_phone in the E164 format, this means that we store both the country code and the national number as a single string. Unfortunately, it looks like the country guessing mechanism of PhoneNumberInput is not as robust as the mechanism from intl-tel-input. For example, using a Stripe UK account, I was able to save a valid UK phone number, however, when the page is refreshed, the system needs to guess the country based on the number alone. Since +44 is used by other countries, the library is guessing the country incorrectly, causing a validation error to appear even if the country is valid:

This is what is saved (note that I'm using a fake phone number for testing purposes): image

This is what happens after I reload: image

As you can see, an incorrect country is guessed, since that country happens to have different validation rules for phone numbers, it automatically causes an error in the page and disables the "Save Changes" button.

With that in mind, I think we should close this issue or defer it until the country guessing in PhoneNumberInput improves.

@FangedParakeet what do you think?

FangedParakeet commented 3 weeks ago

Unfortunately, it looks like the country guessing mechanism of PhoneNumberInput is not as robust as the mechanism from intl-tel-input. For example, using a Stripe UK account, I was able to save a valid UK phone number, however, when the page is refreshed, the system needs to guess the country based on the number alone. Since +44 is used by other countries, the library is guessing the country incorrectly, causing a validation error to appear even if the country is valid...

@mordeth, any thoughts on the comment above? Is there a workaround here or is PhoneNumberInput simply incompatible with our use-cases as currently implemented?