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

phone number format as option instead of hard set to NATIONAL #173

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

Conversation

pierreboissinot
Copy link

behavior like single_text widget)

@pierreboissinot
Copy link
Author

Resolves #171

@pierreboissinot
Copy link
Author

Hi, the travis configuration seems broken

@robhogan
Copy link
Member

robhogan commented Mar 9, 2018

Thanks for the PR. Travis seems to be fine, but this change is causing tests to fail because you’ve changed the default format to INTERNATIONAL.

@pierreboissinot
Copy link
Author

Ok thanks @rh389 I'm updating the test case.

@pierreboissinot
Copy link
Author

@rh389 tests passed =).

public function __construct(array $countryChoices)
public function __construct(
array $countryChoices,
$format = PhoneNumberFormat::INTERNATIONAL
Copy link
Member

@robhogan robhogan Mar 9, 2018

Choose a reason for hiding this comment

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

Sorry, I wasn't clear before. The test cases were fine, they highlighted a breaking change. The problem is this line - the default option should be NATIONAL because that's what it was fixed to before.

Anything else is going to result in a change in behaviour for peoples' existing setups.

Copy link
Author

Choose a reason for hiding this comment

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

Ok i revert test changes and set default format to national, thank you @rh389

Copy link
Member

Choose a reason for hiding this comment

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

If you could add a few more tests to those cases to check the desired result with a couple of different formats, that'd be perfect. Thanks again!

@pierreboissinot
Copy link
Author

@rh389 default format set to NATIONAL

@pierreboissinot
Copy link
Author

Hi @rh389 , I added some tests and a "hack" to prevent any regression: the form option format depends on widget. See 8cd7a42

@pierreboissinot
Copy link
Author

hi @rh389 , do you validate the tests ?

@jkabat
Copy link

jkabat commented Nov 5, 2019

@rh389 ping

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

4 participants