Skip to content

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.

4 participants