Skip to content
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

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

Open
mordeth opened this issue Apr 5, 2024 · 8 comments · May be fixed by #9155
Open

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

mordeth opened this issue Apr 5, 2024 · 8 comments · May be fixed by #9155
Assignees
Labels
category: core WC Payments core related issues, where it’s obvious. focus: checkout payments needs feedback The issue/PR needs a response from any of the parties involved in the issue. priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. type: enhancement The issue is a request for an enhancement.

Comments

@mordeth
Copy link
Contributor

mordeth commented Apr 5, 2024

Description

A new lightweight woocommerce/woocommerce#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:

  • Checkout page
  • Card reader settings

Acceptance criteria

  • Replace the intl-tel-input component with the new WooCommerce component.
  • Eliminate all occurrences of the intl-tel-input component from the codebase.
@anu-rock
Copy link
Contributor

anu-rock commented Apr 5, 2024

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

@mordeth
Copy link
Contributor Author

mordeth commented Apr 5, 2024

The two instances where the old component exists are:

  • Checkout page
  • Card reader settings

I have described them in the issue description.

@RadoslavGeorgiev RadoslavGeorgiev added type: enhancement The issue is a request for an enhancement. category: core WC Payments core related issues, where it’s obvious. focus: checkout payments labels Apr 19, 2024
@RadoslavGeorgiev
Copy link
Contributor

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.

@pierorocca pierorocca added the priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. label Apr 21, 2024
@danielmx-dev
Copy link
Contributor

Please add your planning poker estimate with Zenhub @FangedParakeet

@danielmx-dev danielmx-dev self-assigned this Jul 11, 2024
@danielmx-dev
Copy link
Contributor

danielmx-dev commented Jul 16, 2024

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 #9099):

  • There are some problems when trying to import the CurrencyFactory function exported by @woocommerce/currency @ ver 4.2.0. In order to solve the issue we'd need to upgrade the package to ver 4.3.0, which fixes the issues with the exports; however, this new version requires node 20> as its engine. Doing so would require to update several of our dependencies to satisfy this condition. (At least if we want to continue using engine-strict=true).
    • When I tried to upgrade the engine to node 20> I had to solve a significant amount of issues involving dependencies and tests, which would significantly increase the scope. An upgrade to node 20 was briefly discussed in the past p1706179686901949-slack-CGGCLBN58 but it doesn't seem to be currently in progress. The only related issue I could find related to the upgrade was GitHub Actions - Update workflows using Node JS 16 actions to support Node JS 20 #8129
  • @woocommerce/components does not provide a top level export for the validatePhoneNumber function, so I have to reference the full path, although in my manual testing this hasn't caused any issues so far.
  • PhoneNumberInput doesn't forward some properties like aria-label and placeholder. Supporting these would require to publish a new version of @woocommerce/components

I think we have two options here:

  1. Move forward with the proposed changes in the draft PR Update: Replace intl-tel-input with new PhoneNumberInput component #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 GitHub Actions - Update workflows using Node JS 16 actions to support Node JS 20 #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
Copy link
Contributor

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 #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
Copy link
Contributor

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

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?

@FangedParakeet FangedParakeet added status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. needs feedback The issue/PR needs a response from any of the parties involved in the issue. labels Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: core WC Payments core related issues, where it’s obvious. focus: checkout payments needs feedback The issue/PR needs a response from any of the parties involved in the issue. priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants