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

Bump to support intl tel input 21 #54

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tlebreton
Copy link
Contributor

Hi :)

This pr allow us to use intl-tel-input 21 Changelog 20 Changelog 21

I don't think verison 21 is a big problem for your package (maybe i am wrong :D )

I needed to update test unit because of this :

Remove defaultToFirstCountry option as that behaviour was jackocnr/intl-tel-input#1525 (comment) and so is not recommended (you can always use initialCountry to set the initial country if you wish to)

I decide to add initialCountry: "ch" because it solve test but don't know if it's correct or not.

Another thinks that changes is the way validNumber works :

By default, calling isValidNumber will now default to mobile-only mode (it will only return true for valid mobile numbers), which means it will be jackocnr/intl-tel-input#1535 - if you don't want this, you can pass false as an argument e.g. isValidNumber(false)

It can be solved with another options 'isMobileVerification' with default to true(like intl-tel-input)

What is your opinion on this subject ?

@mpalourdio
Copy link
Owner

mpalourdio commented Apr 4, 2024

I needed to update test unit because of this :

Remove defaultToFirstCountry option as that behaviour was jackocnr/intl-tel-input#1525 (comment) and so is not recommended (you can always use initialCountry to set the initial country if you wish to)

I decide to add initialCountry: "ch" because it solve test but don't know if it's correct or not.

i'll take a look at this change, thanks for pointing it to me.

Another thinks that changes is the way validNumber works :

By default, calling isValidNumber will now default to mobile-only mode (it will only return true for valid mobile numbers), which means it will be jackocnr/intl-tel-input#1535 - if you don't want this, you can pass false as an argument e.g. isValidNumber(false)

That's not how I understand the doc

If you're only dealing with mobile numbers, you can pass true as the first argument to isValidNumber to only validate using mobile number rules, so for the UK, it will only return true for numbers with 11 digits

I suppose we should had a new @Input to handle this bool, and decide with default value we give it.

@tlebreton
Copy link
Contributor Author

tlebreton commented Apr 4, 2024

Yeah you are right i will come back to you with a proposition.

I see that with the last version intl-tel-input is bundled with types so i took the liberty to replace your types by the type of intl-tel-input project.

@tlebreton
Copy link
Contributor Author

So as said before i replace your types by the types from intl-tel-input.

I add a new Input variable isMobileOnly default true (like i understand from the doc)

@Input() required!: boolean;
@Input() isMobileOnly: boolean = true;
Copy link
Owner

@mpalourdio mpalourdio Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in a dedicated commit, and tests should be added.Also, I think that it should be false by default to be the least breaking as possible

import intlTelInput from 'intl-tel-input';
import { IntlTelInputOptions } from '../model/intl-tel-input-options';
import { IntlTelInput } from "../model/intl-tel-input";
import intlTelInput, {Iti, SomeOptions} from 'intl-tel-input';
Copy link
Owner

@mpalourdio mpalourdio Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With v21.0.6, tsconfig.json needs to be changed, like this

    "paths": {
      "intl-tel-input": [
        "node_modules/intl-tel-input/build/js/intlTelInput"
      ]
    },

and old path must be removed

    "paths": {
      "intl-tel-input": [
        "src/lib/@types/intl-tel-input"
      ]
    }

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an error from upstream, will open a PR for discussion. We shouldn't need that

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

@mpalourdio mpalourdio Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

21.0.8+ fixes this, so not needed (old path must still be cleaned up). You may npm update anyway, and see how it goes.

@@ -43,6 +43,7 @@ describe('IntlTelInputComponent', () => {

it('should convert phone number to E164 format', () => {
component.options = {
initialCountry: 'ch',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can't do better, uh

@@ -60,6 +61,7 @@ See the [intl-tel-input repository](https://github.com/jackocnr/intl-tel-input)
i18n: { ch: 'Suisse' },
onlyCountries: ['fr', 'ch']
}"
[isMobileOnly]=false
Copy link
Owner

@mpalourdio mpalourdio Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please reindent and set to true as we'll switch to false by default

@mpalourdio mpalourdio self-assigned this Apr 7, 2024
@mpalourdio
Copy link
Owner

mpalourdio commented Apr 7, 2024

@tlebreton also please run npm update and commit latest lock file, so we are sure tests run with latest version (21.0.7 already). They and are ok at the moment with 21.0.0, but not with 21.0.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants