taiga-family / taiga-ui

Angular UI Kit and components library for awesome people
https://taiga-ui.dev
Apache License 2.0
3.23k stars 449 forks source link

🚀 - InputPhoneInternational component with phone number & type validation #917

Closed rvalitov closed 4 months ago

rvalitov commented 3 years ago

Which @taiga-ui/* package(s) are relevant/releated to the feature request?

core, i18n

Description

Problem 1. Validation and trust of data used in Taiga

It's not specified what source is used for internalization and phone masks in Taiga and if it can be trusted. According to my tests I faced an issue with incorrect phone mask #863. I tried to make a little PR #868 to fix that. But later I found much more inconsistencies with other countries, too. Besides, there are issues with invalid ISO codes, #915, #916.

Problem 2. Phone mask and validation

It's not the best approach to use the mask constants as the are now. The reason is that in some countries the phone number length is different for mobiles and landline phones. Also, some countries have strict rules about the phone numbers, for example, mobile phones can start only with digit 5, and landlines with digit 4. All other prefixes are invalid. Therefore, even if the input is correct in term of length, it's still invalid phone number.

How to solve that?

I think Taiga should use a trusted source of data for phone number masks & validation, and internalization, i.e. include and use some other package that keeps updated when ISO codes change, etc.

My solution

I recommend to use libphonenumber-js:

Because:

Demo

I modified the InputPhoneInternational component to work with libphonenumber-js. The modification includes:

GIF demo:

phoneinput

I think there's a plenty room for discussion here. I'm eager to make a PR.

waterplea commented 3 years ago

The data was given to us by internal analysts long time ago, so I guess it changed since then. I believe it should just be tokenized, it's a mystery to me why it wasn't done in the first place :) Will you be willing to submit a PR? The best resolution would be to sync current constant value with the lib you suggested and use it as a default value for the token. And an info on how to change that to other library should be added to the demo page.

Another point to consider is deprecating built-in values in favor of 3rd party solution, not sure though if it's a good idea not to bundle anything. We definitely not going to add a dependency to main packages. Perhaps we can move these components to a separate addon and add dependency + validators you displayed. So that's something to consider as well.

rvalitov commented 2 years ago

I can make a PR. Perhaps, the approach with separate addon & dependency there is the best. In this case the InputPhoneInternational moves completely to the addon? Or there will be 2 versions of it: as addon, and simple bundled with the core?

waterplea commented 2 years ago

I don't really like to put dependencies and make more packages under the hood of this repo. Would you be interested in making your own separate package for Taiga? Ideally I see it like this:

  1. ​A PR to fix current list and make a token for the list.
  2. A separate package with libphonenumber-js that has a new value for that token and a validator and whatever else you see helpful
  3. We can then put a link to that package on the docs page for InputPhoneInternational
  4. Perhaps we can remove default value for the token in 3.0 citing your package as recommended solution and this way we will make bundle a little bit smaller for people who don't need all countries
rvalitov commented 2 years ago

Not sure if it's related, sometimes in console of the Taiga manual site I see errors:

Failed to load resource: the server responded with a status of 404 ()
main-es2015.35912c4ae7216946e268.js:1 ERROR Error: Uncaught (in promise): ChunkLoadError: Loading chunk 43 failed.
(error: https://taiga-ui.dev/43-es2015.53efa5f66ee8aeabfce1.js)
ChunkLoadError: Loading chunk 43 failed.
(error: https://taiga-ui.dev/43-es2015.53efa5f66ee8aeabfce1.js)
    at Function.r.e (runtime-es2015.d4890cf23e46165775ab.js:1)
    at loadChildren (main-es2015.35912c4ae7216946e268.js:2)
    at Xt.loadModuleFactory (main-es2015.35912c4ae7216946e268.js:2)
    at Xt.load (main-es2015.35912c4ae7216946e268.js:2)
    at u.project (main-es2015.35912c4ae7216946e268.js:2)
    at u._tryNext (main-es2015.35912c4ae7216946e268.js:1)
    at u._next (main-es2015.35912c4ae7216946e268.js:1)
    at u.next (main-es2015.35912c4ae7216946e268.js:1)
    at e._subscribe (main-es2015.35912c4ae7216946e268.js:1)
    at e._trySubscribe (main-es2015.35912c4ae7216946e268.js:1)
    at w (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at w (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at polyfills-es2015.74b69fa587adac4cf724.js:1
    at u.invokeTask (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at Object.onInvokeTask (main-es2015.35912c4ae7216946e268.js:1)
    at u.invokeTask (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at a.runTask (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at m (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at l.invokeTask [as invoke] (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at h (polyfills-es2015.74b69fa587adac4cf724.js:1)
Xn @ main-es2015.35912c4ae7216946e268.js:1
common-es2015.c78a287d4c889cfd8725.js:1 Failed to load resource: the server responded with a status of 404 ()
0-es2015.f13e589447611031432e.js:1 Failed to load resource: the server responded with a status of 404 ()
0-es2015.f13e589447611031432e.js:1 Failed to load resource: the server responded with a status of 404 ()
main-es2015.35912c4ae7216946e268.js:1 ERROR Error: Uncaught (in promise): ChunkLoadError: Loading chunk 0 failed.
(error: https://taiga-ui.dev/0-es2015.f13e589447611031432e.js)
ChunkLoadError: Loading chunk 0 failed.
(error: https://taiga-ui.dev/0-es2015.f13e589447611031432e.js)
    at Function.r.e (runtime-es2015.d4890cf23e46165775ab.js:1)
    at loadChildren (main-es2015.35912c4ae7216946e268.js:2)
    at Xt.loadModuleFactory (main-es2015.35912c4ae7216946e268.js:2)
    at Xt.load (main-es2015.35912c4ae7216946e268.js:2)
    at u.project (main-es2015.35912c4ae7216946e268.js:2)
    at u._tryNext (main-es2015.35912c4ae7216946e268.js:1)
    at u._next (main-es2015.35912c4ae7216946e268.js:1)
    at u.next (main-es2015.35912c4ae7216946e268.js:1)
    at e._subscribe (main-es2015.35912c4ae7216946e268.js:1)
    at e._trySubscribe (main-es2015.35912c4ae7216946e268.js:1)
    at w (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at w (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at polyfills-es2015.74b69fa587adac4cf724.js:1
    at u.invokeTask (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at Object.onInvokeTask (main-es2015.35912c4ae7216946e268.js:1)
    at u.invokeTask (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at a.runTask (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at m (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at l.invokeTask [as invoke] (polyfills-es2015.74b69fa587adac4cf724.js:1)
    at h (polyfills-es2015.74b69fa587adac4cf724.js:1)
Xn @ main-es2015.35912c4ae7216946e268.js:1
common-es2015.c78a287d4c889cfd8725.js:1 Failed to load resource: the server responded with a status of 404 ()
45-es2015.083c5b2a9c7777b1e7a1.js:1 Failed to load resource: the server responded with a status of 404 ()
styles.cd96327f16f5dc487bb7.css:1 Failed to load resource: the server responded with a status of 404 ()
icons:1 Failed to load resource: the server responded with a status of 404 ()
waterplea commented 2 years ago

Looks like a lazy loaded chunk fail because new content was deployed while you were on one page and the cache busting id has changed.

waterplea commented 2 years ago

@rvalitov what do you think about my idea of a separate package, providing countries from libphonenumber-js? Are you interested in making one?

rvalitov commented 2 years ago

@waterplea Yes, I'm interested for sure. Besides, I already have a solution I use that I just need to rewrite to make it as a package. Perhaps, you could give me some tips how to make a package, because I never done that before. However, currently I'm very busy finalizing my projects at work. If it's not urgent, may I return to this issue in January?

waterplea commented 2 years ago

Sure. We could work on this together, we have an angular starter that we can use, but we wanted to update it for a while now. I think we can try doing that in January and test run it on that package.

waterplea commented 2 years ago

@rvalitov are you still interested in this one? We've updated our starter: https://github.com/Tinkoff/angular-open-source-starter