Skip to content

[Form] DateIntervalType: Allow to configure labels & enhance form theme #20887

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 11, 2017
+122 −16
Merged

[Form] DateIntervalType: Allow to configure labels & enhance form theme #20887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,32 @@
{%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-inline')|trim}) -%}
<div {{ block('widget_container_attributes') }}>
{{- form_errors(form) -}}
{%- if with_years %}{{ form_widget(form.years) }}{% endif -%}
{%- if with_months %}{{ form_widget(form.months) }}{% endif -%}
{%- if with_weeks %}{{ form_widget(form.weeks) }}{% endif -%}
{%- if with_days %}{{ form_widget(form.days) }}{% endif -%}
{%- if with_hours %}{{ form_widget(form.hours) }}{% endif -%}
{%- if with_minutes %}{{ form_widget(form.minutes) }}{% endif -%}
{%- if with_seconds %}{{ form_widget(form.seconds) }}{% endif -%}
<div class="table-responsive">
<table class="table {{ table_class|default('table-bordered table-condensed table-striped') }}">
<thead>
<tr>
{%- if with_years %}<th>{{ form_label(form.years) }}</th>{% endif -%}
{%- if with_months %}<th>{{ form_label(form.months) }}</th>{% endif -%}
{%- if with_weeks %}<th>{{ form_label(form.weeks) }}</th>{% endif -%}
{%- if with_days %}<th>{{ form_label(form.days) }}</th>{% endif -%}
{%- if with_hours %}<th>{{ form_label(form.hours) }}</th>{% endif -%}
{%- if with_minutes %}<th>{{ form_label(form.minutes) }}</th>{% endif -%}
{%- if with_seconds %}<th>{{ form_label(form.seconds) }}</th>{% endif -%}
</tr>
</thead>
<tbody>
<tr>
{%- if with_years %}<td>{{ form_widget(form.years) }}</td>{% endif -%}
{%- if with_months %}<td>{{ form_widget(form.months) }}</td>{% endif -%}
{%- if with_weeks %}<td>{{ form_widget(form.weeks) }}</td>{% endif -%}
{%- if with_days %}<td>{{ form_widget(form.days) }}</td>{% endif -%}
{%- if with_hours %}<td>{{ form_widget(form.hours) }}</td>{% endif -%}
{%- if with_minutes %}<td>{{ form_widget(form.minutes) }}</td>{% endif -%}
{%- if with_seconds %}<td>{{ form_widget(form.seconds) }}</td>{% endif -%}
</tr>
</tbody>
</table>
</div>
{%- if with_invert %}{{ form_widget(form.invert) }}{% endif -%}
</div>
{%- endif -%}
Expand Down
31 changes: 24 additions & 7 deletions src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,30 @@
{%- else -%}
<div {{ block('widget_container_attributes') }}>
{{- form_errors(form) -}}
{%- if with_years %}{{ form_widget(form.years) }}{% endif -%}
{%- if with_months %}{{ form_widget(form.months) }}{% endif -%}
{%- if with_weeks %}{{ form_widget(form.weeks) }}{% endif -%}
{%- if with_days %}{{ form_widget(form.days) }}{% endif -%}
{%- if with_hours %}{{ form_widget(form.hours) }}{% endif -%}
{%- if with_minutes %}{{ form_widget(form.minutes) }}{% endif -%}
{%- if with_seconds %}{{ form_widget(form.seconds) }}{% endif -%}
<table class="{{ table_class|default('') }}">
<thead>
<tr>
{%- if with_years %}<th>{{ form_label(form.years) }}</th>{% endif -%}
{%- if with_months %}<th>{{ form_label(form.months) }}</th>{% endif -%}
{%- if with_weeks %}<th>{{ form_label(form.weeks) }}</th>{% endif -%}
{%- if with_days %}<th>{{ form_label(form.days) }}</th>{% endif -%}
{%- if with_hours %}<th>{{ form_label(form.hours) }}</th>{% endif -%}
{%- if with_minutes %}<th>{{ form_label(form.minutes) }}</th>{% endif -%}
{%- if with_seconds %}<th>{{ form_label(form.seconds) }}</th>{% endif -%}
</tr>
</thead>
<tbody>
<tr>
{%- if with_years %}<td>{{ form_widget(form.years) }}</td>{% endif -%}
{%- if with_months %}<td>{{ form_widget(form.months) }}</td>{% endif -%}
{%- if with_weeks %}<td>{{ form_widget(form.weeks) }}</td>{% endif -%}
{%- if with_days %}<td>{{ form_widget(form.days) }}</td>{% endif -%}
{%- if with_hours %}<td>{{ form_widget(form.hours) }}</td>{% endif -%}
{%- if with_minutes %}<td>{{ form_widget(form.minutes) }}</td>{% endif -%}
{%- if with_seconds %}<td>{{ form_widget(form.seconds) }}</td>{% endif -%}
</tr>
</tbody>
</table>
{%- if with_invert %}{{ form_widget(form.invert) }}{% endif -%}
</div>
{%- endif -%}
Expand Down
25 changes: 23 additions & 2 deletions src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,10 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$childOptions = array();
foreach ($this->timeParts as $part) {
if ($options['with_'.$part]) {
$childOptions[$part] = array();
$childOptions[$part]['error_bubbling'] = true;
$childOptions[$part] = array(
'error_bubbling' => true,
'label' => $options['labels'][$part],
);
if ('choice' === $options['widget']) {
$childOptions[$part]['choice_translation_domain'] = false;
$childOptions[$part]['choices'] = $options[$part];
Expand Down Expand Up @@ -131,6 +133,7 @@ public function buildForm(FormBuilderInterface $builder, array $options)
}
if ($options['with_invert']) {
$builder->add('invert', 'Symfony\Component\Form\Extension\Core\Type\CheckboxType', array(
'label' => $options['labels']['invert'],
'error_bubbling' => true,
'required' => false,
'translation_domain' => $options['translation_domain'],
Expand Down Expand Up @@ -192,6 +195,21 @@ public function configureOptions(OptionsResolver $resolver)
return array_fill_keys($timeParts, $placeholder);
};

$labelsNormalizer = function (Options $options, array $labels) {
return array_replace(array(
'years' => null,
'months' => null,
'days' => null,
'weeks' => null,
'hours' => null,
'minutes' => null,
'seconds' => null,
'invert' => 'Negative interval',
), array_filter($labels, function ($label) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified by array_filter($labels).

Copy link
Contributor Author

@ogizanagi ogizanagi Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

No, because it'll filter any empty value (actually more accurratly: any entry that would be converted to false).
In case someone simply want to disable the label, he'll set it to false. But false would be removed and thus would make it impossible to disable the label rendering from options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HeahDude : Do you think gathering the labels under a single option is a good idea or should I opt back for label_x options?

#21002 is suggesting to translate DateTimeType subtypes labels and uses dedicated options. I guess it's coherent regarding other options actually, even if it'll looks ugly in the case of the DateIntervalType. But I guess we should be consistent. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer your approach :). Problem is that it mostly depends on the widget we consider.

Having the possibility to define labels is great, mainly to translate them. It means we should be able to define all of them (and that would be consistent).

Also regarding performances, your approach should be better, using one normalizer for one array (even calling an array_replace and an array_filter) instead of defining many options. Resolving only one option involves cloning the resolver instance and many calls, so that must cost much more.

In conclusion, I'd say maybe #20002 should follow what's implemented here, using date_label and time_label as string or array, so maybe DateType and TimeType should have a labels option too.

What do you and others think?

Copy link
Contributor Author

@ogizanagi ogizanagi Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I opted for this approach as it was my own preference, but expected negative feedbacks about it. Apparently it doesn't look that bad, so I'm satisfied 😅 .

Apart from perfs, IMHO segregating similar options is cluttering form types and makes documentation more complicated and verbose for no gain. Of course it depends of use cases and options, as it makes sense for most of them to be separated from each others in order to benefits of fine control over OptionResolver features like type/values validation and normalization. But even for some of them, a feature like #18134 would be interesting (with an extra cost, though).

My only fear was about consistency with other form types which opted for segregation and autocompletions with the Symfony plugin for PHPStorm for instance.

return null !== $label;
}));
};

$resolver->setDefaults(
array(
'with_years' => true,
Expand Down Expand Up @@ -220,9 +238,11 @@ public function configureOptions(OptionsResolver $resolver)
// this option.
'data_class' => null,
'compound' => $compound,
'labels' => array(),
)
);
$resolver->setNormalizer('placeholder', $placeholderNormalizer);
$resolver->setNormalizer('labels', $labelsNormalizer);

$resolver->setAllowedValues(
'input',
Expand Down Expand Up @@ -260,6 +280,7 @@ public function configureOptions(OptionsResolver $resolver)
$resolver->setAllowedTypes('with_minutes', 'bool');
$resolver->setAllowedTypes('with_seconds', 'bool');
$resolver->setAllowedTypes('with_invert', 'bool');
$resolver->setAllowedTypes('labels', 'array');
}

/**
Expand Down
49 changes: 49 additions & 0 deletions src/Symfony/Component/Form/Tests/Extension/Core/Type/DateIntervalTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,4 +399,53 @@ public function testInvertDoesNotInheritRequiredOption()
);
$this->assertFalse($form->get('invert')->getConfig()->getOption('required'));
}

public function testCanChangeTimeFieldsLabels()
{
$form = $this->factory->create(
DateIntervalType::class,
null,
array(
'required' => true,
'with_invert' => true,
'with_hours' => true,
'with_minutes' => true,
'with_seconds' => true,
'labels' => array(
'invert' => 'form.trans.invert',
'years' => 'form.trans.years',
'months' => 'form.trans.months',
'days' => 'form.trans.days',
'hours' => 'form.trans.hours',
'minutes' => 'form.trans.minutes',
'seconds' => 'form.trans.seconds',
),
)
);

$view = $form->createView();
$this->assertSame('form.trans.invert', $view['invert']->vars['label']);
$this->assertSame('form.trans.years', $view['years']->vars['label']);
$this->assertSame('form.trans.months', $view['months']->vars['label']);
$this->assertSame('form.trans.days', $view['days']->vars['label']);
$this->assertSame('form.trans.hours', $view['hours']->vars['label']);
$this->assertSame('form.trans.minutes', $view['minutes']->vars['label']);
$this->assertSame('form.trans.seconds', $view['seconds']->vars['label']);
}

public function testInvertDefaultLabel()
{
$form = $this->factory->create(DateIntervalType::class, null, array('with_invert' => true));

$view = $form->createView();
$this->assertSame('Negative interval', $view['invert']->vars['label']);

$form = $this->factory->create(DateIntervalType::class, null, array(
'with_invert' => true,
'labels' => array('invert' => null),
));

$view = $form->createView();
$this->assertSame('Negative interval', $view['invert']->vars['label']);
}
}