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

DateIntervalType: 'invert' should not inherit the 'required' option #20877

Closed
wants to merge 3 commits into
base: 3.2
from

Conversation

Projects
None yet
5 participants
@galeaspablo
Contributor

galeaspablo commented Dec 12, 2016

Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20876
License MIT
Doc PR -

As explained in #20876,

In the DateIntervalType, there is a field, called 'invert', that allows for negative intervals. This is outputted as a checkbox. Which is fine, but it shouldn't be required.

@@ -134,6 +134,8 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$builder->add($childForm);
}
}
// Invert should not inherit the required option
$invertOptions['required'] = false;

This comment has been minimized.

@ogizanagi

ogizanagi Dec 12, 2016

Member

What about

diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php
diff --git a/src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php b/src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php
index fb2b7d7..e4f46d8 100644
--- a/src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php
+++ b/src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php
@@ -107,9 +107,6 @@ class DateIntervalType extends AbstractType
                     }
                 }
             }
-            $invertOptions = array(
-                'error_bubbling' => true,
-            );
             // Append generic carry-along options
             foreach (array('required', 'translation_domain') as $passOpt) {
                 foreach ($this->timeParts as $part) {
@@ -117,10 +114,8 @@ class DateIntervalType extends AbstractType
                         $childOptions[$part][$passOpt] = $options[$passOpt];
                     }
                 }
-                if ($options['with_invert']) {
-                    $invertOptions[$passOpt] = $options[$passOpt];
-                }
             }
+
             foreach ($this->timeParts as $part) {
                 if ($options['with_'.$part]) {
                     $childForm = $builder->create($part, self::$widgets[$options['widget']], $childOptions[$part]);
@@ -135,7 +130,11 @@ class DateIntervalType extends AbstractType
                 }
             }
             if ($options['with_invert']) {
-                $builder->add('invert', 'Symfony\Component\Form\Extension\Core\Type\CheckboxType', $invertOptions);
+                $builder->add('invert', 'Symfony\Component\Form\Extension\Core\Type\CheckboxType', array(
+                    'error_bubbling' => true,
+                    'required' => false,
+                    'translation_domain' => $options['translation_domain'],
+                ));
             }
             $builder->addViewTransformer(new DateIntervalToArrayTransformer($parts, 'text' === $options['widget']));
         }

instead? Not much different, but simplify things a little bit.

This comment has been minimized.

@galeaspablo

galeaspablo Dec 12, 2016

Contributor

Looks good to me. Changed. 👍

}

This comment has been minimized.

@ogizanagi

ogizanagi Dec 12, 2016

Member

Should be reverted

This comment has been minimized.

@galeaspablo

galeaspablo Dec 12, 2016

Contributor

Hehe, I took your comment to the dot. No worries, removed the space now.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Dec 12, 2016

👍

Status: Reviewed

$builder->add('invert', 'Symfony\Component\Form\Extension\Core\Type\CheckboxType', array(
'error_bubbling' => true,
'required' => false,
'translation_domain' => $options['translation_domain'],

This comment has been minimized.

@xabbuh

xabbuh Dec 12, 2016

Member

Is the translation_domain option needed here at all?

This comment has been minimized.

@galeaspablo

galeaspablo Dec 12, 2016

Contributor

It's the previous behavior, not an addition. Removing that should be a separate issue.

This comment has been minimized.

@ogizanagi

ogizanagi Dec 12, 2016

Member

Anyway it should be kept IMHO. It's used to translate the Invert label, so it makes sense to use the same translation domain as for the whole widget.

This comment has been minimized.

@xabbuh

xabbuh Dec 13, 2016

Member

@ogizanagi Indeed, you are right.

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 13, 2016

Thank you @galeaspablo.

fabpot added a commit that referenced this pull request Dec 13, 2016

bug #20877 DateIntervalType: 'invert' should not inherit the 'require…
…d' option (galeaspablo)

This PR was squashed before being merged into the 3.2 branch (closes #20877).

Discussion
----------

DateIntervalType: 'invert' should not inherit the 'required' option

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20876
| License       | MIT
| Doc PR        | -

As explained in #20876,

> In the DateIntervalType, there is a field, called 'invert', that allows for negative intervals. This is outputted as a checkbox. Which is fine, but it shouldn't be required.

Commits
-------

b1597f1 DateIntervalType: 'invert' should not inherit the 'required' option

@fabpot fabpot closed this Dec 13, 2016

@fabpot fabpot referenced this pull request Dec 13, 2016

Merged

Release v3.2.1 #20897

fabpot added a commit that referenced this pull request Jan 11, 2017

feature #20887 [Form] DateIntervalType: Allow to configure labels & e…
…nhance form theme (ogizanagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

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

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no (unless someone relies on this non themed type)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | Should document the new `labels` option

I just realized by using it for last fixes in #20886 and #20877 that this type was not really themed:

### before

<img width="861" alt="screenshot 2016-12-13 a 00 54 35" src="https://cloud.githubusercontent.com/assets/2211145/21121792/c589d27a-c0ce-11e6-8368-a396fda3bc7a.PNG">

At least labels should appear, but this also means being able to change them (thus the new `labels` option).

I think the form themes should provide a functional & minimalistic integration like this:

### after

<img width="862" alt="screenshot 2016-12-13 a 00 54 17" src="https://cloud.githubusercontent.com/assets/2211145/21121814/d9c4ead6-c0ce-11e6-94e1-41e6c14884a7.PNG">

---
(On screenshots above, I've only added a css rule to remove the 100% width of the `.table` class. See #20887 (comment))

Commits
-------

bfd9e50 [Form] DateIntervalType: Allow to configure labels & enhance form theme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment