-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: Add a feature to change country display format in form #95
Changes from all commits
82f7a1a
fcff4cc
4af7793
b852427
00d10e3
534552b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,9 @@ class PhoneNumberType extends AbstractType | |
public const WIDGET_SINGLE_TEXT = 'single_text'; | ||
public const WIDGET_COUNTRY_CHOICE = 'country_choice'; | ||
|
||
public const DISPLAY_COUNTRY_FULL = 'display_country_full'; | ||
public const DISPLAY_COUNTRY_SHORT = 'display_country_short'; | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
|
@@ -60,13 +63,15 @@ public function buildForm(FormBuilderInterface $builder, array $options) | |
} | ||
|
||
$countryChoices = []; | ||
$intlCountries = Countries::getNames(); | ||
|
||
foreach (Countries::getNames() as $region => $name) { | ||
if (false === isset($countries[$region])) { | ||
foreach ($countries as $regionCode => $countryCode) { | ||
if (!isset($intlCountries[$regionCode])) { | ||
continue; | ||
} | ||
|
||
$countryChoices[sprintf('%s (+%s)', $name, $countries[$region])] = $region; | ||
$label = $this->formatDisplayChoice($options['country_display_type'], $intlCountries[$regionCode], $regionCode, $countryCode); | ||
$countryChoices[$label] = $regionCode; | ||
} | ||
|
||
$transformerChoices = array_values($countryChoices); | ||
|
@@ -128,6 +133,7 @@ public function configureOptions(OptionsResolver $resolver) | |
'by_reference' => false, | ||
'error_bubbling' => false, | ||
'country_choices' => [], | ||
'country_display_type' => self::DISPLAY_COUNTRY_FULL, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a break, we currently use short format, isn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nop, currently we use "France (+33)" the "FULL" format There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad |
||
'country_placeholder' => false, | ||
'preferred_country_choices' => [], | ||
'country_options' => [], | ||
|
@@ -139,6 +145,11 @@ public function configureOptions(OptionsResolver $resolver) | |
self::WIDGET_COUNTRY_CHOICE, | ||
]); | ||
|
||
$resolver->setAllowedValues('country_display_type', [ | ||
self::DISPLAY_COUNTRY_FULL, | ||
self::DISPLAY_COUNTRY_SHORT, | ||
]); | ||
|
||
$resolver->setAllowedTypes('country_options', 'array'); | ||
$resolver->setAllowedTypes('number_options', 'array'); | ||
} | ||
|
@@ -158,4 +169,13 @@ public function getBlockPrefix() | |
{ | ||
return 'phone_number'; | ||
} | ||
|
||
private function formatDisplayChoice(string $displayType, string $regionName, string $regionCode, string $countryCode): string | ||
{ | ||
if (self::DISPLAY_COUNTRY_SHORT === $displayType) { | ||
return sprintf('%s +%s', $regionCode, $countryCode); | ||
} | ||
|
||
return sprintf('%s (+%s)', $regionName, $countryCode); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? It's not equivalent to what was here before. Therefore it potentially breaks the compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results is almost the same. I made this change to keep the order given in the
countries
configuration entry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, the performances are not the same this way because before the data was loaded and browsed only once in the buffer of the component while now we have it as many times as countries support x2 (exists & getName)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right ! Not sure its a big deal. Let me check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not be so convinced. I would like to have a verification on this before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance overhead is actually small. But I'm surprised there's indeed a topic here. That said, a solution can be quickly found here:
could be changed easily with
(and
Countries::getName($regionCode)
by$intlCountries[$regionCode]
)