Skip to content

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

Open
pierreboissinot wants to merge 5 commits into
misd-service-development:masterfrom
pierreboissinot:widget-country-choice-with-format-option
Open

phone number format as option instead of hard set to NATIONAL#173
pierreboissinot wants to merge 5 commits into
misd-service-development:masterfrom
pierreboissinot:widget-country-choice-with-format-option

Conversation

@pierreboissinot

Copy link
Copy Markdown

behavior like single_text widget)

@pierreboissinot

Copy link
Copy Markdown
Author

Resolves #171

@pierreboissinot

Copy link
Copy Markdown
Author

Hi, the travis configuration seems broken

@robhogan

robhogan commented Mar 9, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
Author

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

@pierreboissinot

Copy link
Copy Markdown
Author

@rh389 tests passed =).

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

@robhogan robhogan Mar 9, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

@rh389 default format set to NATIONAL

@pierreboissinot

Copy link
Copy Markdown
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
Copy Markdown
Author

hi @rh389 , do you validate the tests ?

@jkabat

jkabat commented Nov 5, 2019

Copy link
Copy Markdown

@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