Do not put any empty div if label is disabled #19247

soullivaneuh · 2016-06-30T15:09:57Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#18850, #18474
License	MIT
Doc PR	N/A

This produces wrong alignments especially for form collections.

This PR is an alternative to #18504, fixing both #18850 and #18474 issues.

@lyrixx what do you think of this? Maybe I'm forgetting some edge cases, please tell me so we can discuss about it! 👍

cc @jongotlin

Application

With the following form type configuration:

$builder
    ->add('name', null, [
        'label' => 'form.name',
        'translation_domain' => 'global',
        'text_suffix' => '@'.$domain->getName(),
    ])
    ->add('destinations', CollectionType::class, [
        'label' => 'domain.form.mailalias_destinations',
        'translation_domain' => 'front',
        'allow_add' => true,
        'allow_delete' => true,
        'prototype' => true,
        'by_reference' => false,
        'entry_options' => [
            'label' => false,
            'text_prefix' => '@',
        ],
    ])
;

The actual rendering looks like this:

Thanks to this PR, it's now:

jvasseur · 2016-06-30T15:13:45Z

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig

+    {% else %}
+        col-sm-10
+    {% endif %}
+{% endspaceless %}


You should use whitespace control modifiers instead of the spaceless tag.

@jvasseur fixed.

HeahDude · 2016-06-30T15:14:14Z

Prettier than #18504, looks great! Maybe a test would be good.

greg0ire · 2016-06-30T15:14:18Z

This looks cleaner than having an empty div.

soullivaneuh · 2016-06-30T15:16:23Z

Maybe a test would be good.

@HeahDude Do we have test suites on Symfony for templates? I would be glad to add some. Could you please indicate where to go?

HeahDude · 2016-06-30T15:23:41Z

Take a look at https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php, https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php, https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php, https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractBootstrap3HorizontalLayoutTest.php.

They are the main files for testing form templating in TwigBridge and FrameworkBundle.

greg0ire · 2016-06-30T15:24:54Z

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig

@@ -77,5 +75,9 @@ col-sm-2
 {% endblock reset_row %}

 {% block form_group_class -%}
-col-sm-10
+    {% if label is same as(false) -%}


Isn't there a need for a minus sign before the if ?

No, there isn't

Thanks, just checking 👍

This is needed for white-space control.

If I remove it, I get:

<div class=" col-sm-12">

Oh you mean for block! Indeed.

Ok you're right @greg0ire, should be:

{%- block form_group_class -%} {%- if label is same as(false) -%} ...

Are you sure ? minus for both block and if ?

Yes this happens in a string not at the start of a block

Ok… anyway I think @soullivaneuh checks what this produces, so…

I did. And the actual state works correctly.

HeahDude · 2016-06-30T15:25:32Z

There is also https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractTableLayoutTest.php but it should not be concerned here. Maybe it is :)

soullivaneuh · 2016-06-30T15:44:38Z

@HeahDude I added a test. Please tell me if it looks OK.

lyrixx · 2016-06-30T15:46:55Z

It looks good to me. But if you can attach screenshot before / after in the PR description, it would be very nice.

greg0ire · 2016-06-30T15:49:41Z

src/Symfony/Component/Form/Tests/AbstractBootstrap3HorizontalLayoutTest.php

+        $form = $this->factory->createNamed('name', 'date', null, array('label' => false));
+        $view = $form->createView();
+        $this->renderWidget($view);
+        $html = $this->renderLabel($view);


Consider not using the $form and $html OTVs

Can be inlined in assertEmpty()

This was just a copy paste of a similar test.

soullivaneuh · 2016-06-30T18:01:08Z

@lyrixx I updated the PR body. 👍

Note: We can have one edge case AFAIK:

->add('test', TextType::class, [
    'mapped' => false,
    'label' => false,
    'attr' => [
        'placeholder' => 'My label goes here',
    ]
])

Renders:

I think this case is very rare comparing to the other, but can still happen.

Maybe could we have an attr "option" like you did with *-inline classes for checkbox_widget and radio_widget?

stof · 2016-06-30T18:04:32Z

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig

-col-sm-10
-{%- endblock form_group_class %}
+{% block form_group_class %}
+    {% if label is same as(false) -%}


missing whitespace control here to remove the indentation

You mean {%- if?

Indeed, good catch!

soullivaneuh · 2016-06-30T18:06:00Z

Also maybe we should create a new block for the form-group class? Because ATM, user will have to copy/paste the whole logic if they want to change it:

{% block form_group_class %}
    {% if label is not same as(false) -%}
        col-md-9
    {%- else -%}
        col-md-12
    {%- endif %}
{% endblock form_group_class %}

Or maybe a form_group_class and form_group_without_label_class?

jongotlin · 2016-07-01T06:23:30Z

My two cents..

Your code will still render the form-group-div. Maybe it's not wrong but having a form-group inside another form-group doesn't look good.
I think it's a BC-break removing the "level one"-label column if the label is false. Even if it's rare doing so we should not change the behavior.

Can confirm the form I used in my pr looks the same with this code. 👍

soullivaneuh · 2016-07-01T07:57:12Z

Your code will still render the form-group-div. Maybe it's not wrong but having a form-group inside another form-group doesn't look good.

The form-group is needed to keep consistent margins. Otherwise, you will get something like this:

Consider this as a row for a form. AFAIK, we can make them nested.

I think it's a BC-break removing the "level one"-label column if the label is false. Even if it's rare doing so we should not change the behavior.

Please see #19247 (comment).

jongotlin · 2016-07-01T08:48:30Z

We're solving two different thing here. Collections and nested forms. Consider my nested form type the form-group-div inside another form-group is not a clean solution.

Form from my pr

<div class="form-group">
    <div class="col-sm-12">
        <div id="foo_foo2">
            <div class="form-group">
                <label class="col-sm-2 control-label required" for="foo_foo2_child">Child</label>
                <div class="col-sm-10">
                    <input type="text" id="foo_foo2_child" name="foo[foo2][child]" required="required" class="form-control" />
                </div>
            </div>
        </div>
    </div>
</div>

When rendering the collection type I see the need for it. Is there a way to know the difference between them?

Regarding BC-breaks. Yes, that comment is what I was thinking of. I don't think we should change the behavior and render the input the whole width.

soullivaneuh · 2016-07-01T09:16:51Z

You mean about:

<div class="form-group">
    <div class="col-sm-12">

?

I don't see the issue. This is like having a .row and a col-sm-12 to me. This does not violate Bootstrap convention AFAIK.

jongotlin · 2016-07-01T09:52:36Z

I mean

Maybe it's not wrong but having a lot of nested form types the markup wont look pretty.

soullivaneuh · 2016-07-01T10:03:00Z

The code would be a lot complexified, not sure it worth it just for an esthetic case...

@lyrixx Could we please have your opinion for this and #19247 (comment)?

jongotlin · 2016-07-01T11:01:09Z

Agreed! I'm more concerned about the BC-break.

HeahDude · 2016-07-28T22:06:01Z

src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig

-{% block form_group_class -%}
-col-sm-10
-{%- endblock form_group_class %}
+{% block form_group_class %}


missing space control at the end

Space control added.

HeahDude · 2016-07-28T22:10:20Z

src/Symfony/Component/Form/Tests/AbstractBootstrap3HorizontalLayoutTest.php

@@ -100,6 +100,16 @@ public function testLabelWithCustomTextAsOptionAndCustomAttributesPassedDirectly
        );
    }

+    public function testLabelNoDivIfFalse()
+    {
+        $this->assertEmpty(


Maybe you could use a consistent style here:

$view = $this->factory->createNamed('name', 'date', null, array('label' => false))->createView(); $this->assertEmpty($this->renderLabel($view), 'The label should not be rendered');

I honestly don't see the interest. You create a variable that will be used only once.

Why not simply pass the result to the function?

I second that. See more information about thoughtless one-time variables here. Note that OTVs can be used to break down complicated expressions, but I personally don't feel this is complicated enough to warrant an OTV.

If you need the variable because it clarifies things, maybe consider using a comment instead, you can even write whole sentences with them.

fabpot · 2016-09-14T22:07:47Z

@soullivaneuh Looks like there aren't that many things to do to finish this one, can you have a look at the latest comments? Thanks.

This produces wrong alignments especially for form collections.

soullivaneuh · 2016-09-15T08:43:46Z

@fabpot I corrected the review and answered for this one: #19247 (comment)

Also I think I could add a commit to improve a little the process.

IMHO, we should introduce some new blocks:

form_group_without_label_class: Class to put when no label
form_group_with_label_class: Class to put with an existing label

With the current state of this PR, if we want to change the col classes like this:

{% use 'bootstrap_3_horizontal_layout.html.twig' %}

{% block form_label_class -%}
    col-sm-3
{%- endblock form_label_class %}

We will have a broken form like this:

So we have to override form_group_class too. But in this case, we have to copy/paste the vendor logic:

{% block form_group_class -%}
    {%- if label is same as(false) -%}
        col-sm-12
    {%- else -%}
        col-sm-9 {# This is the ONLY real override here #}
    {%- endif -%}
{%- endblock form_group_class %}

If we have the two blocks I proposed, this will just looks like this (final state):

{% use 'bootstrap_3_horizontal_layout.html.twig' %}

{% block form_label_class -%}
    col-sm-3
{%- endblock form_label_class %}

{% block form_group_with_label_class -%}
    col-sm-9
{%- endblock form_group_with_label_class %}

What do you think? Maybe we can find a simplier solution for block name, I waiting your feedbacks for that. But I think we really should avoid the ugly copy/paste constraint.

javiereguiluz · 2016-11-07T10:59:18Z

@soullivaneuh would it make sense to only introduce a new block called form_group_without_label_class and keep the existing block for the common case of having a label?

soullivaneuh · 2017-06-27T16:53:09Z

Sorry guys, but I don't have any more time to work on this PR right now.

I allowed edit from maintainer, if anyone want to continue it.

fabpot · 2017-07-06T07:58:05Z

@javiereguiluz @HeahDude Can you take over this one? Or anyone else wanting to take over?

fabpot · 2017-10-01T17:25:29Z

Closing as there is apparently nobody wanting to take over this PR. Feel free to reopen.

soullivaneuh force-pushed the twig-form-bootstrap-empty-label-align branch from ae3f3e1 to aeb4de6 Compare June 30, 2016 15:10

jvasseur reviewed Jun 30, 2016
View reviewed changes

soullivaneuh force-pushed the twig-form-bootstrap-empty-label-align branch from aeb4de6 to e4699e2 Compare June 30, 2016 15:20

javiereguiluz added TwigBridge Bug labels Jun 30, 2016

greg0ire reviewed Jun 30, 2016
View reviewed changes

soullivaneuh force-pushed the twig-form-bootstrap-empty-label-align branch from 830937e to 66000e2 Compare June 30, 2016 15:45

soullivaneuh mentioned this pull request Jun 30, 2016

No Bootstrap markup is rendered for label set as false #18504

Closed

greg0ire reviewed Jun 30, 2016
View reviewed changes

stof reviewed Jun 30, 2016
View reviewed changes

carsonbot added the Status: Needs Review label Jul 1, 2016

HeahDude reviewed Jul 28, 2016
View reviewed changes

Do not put any empty div if label is disabled

6efc6b2

This produces wrong alignments especially for form collections.

soullivaneuh force-pushed the twig-form-bootstrap-empty-label-align branch from 2c059f2 to 6efc6b2 Compare September 15, 2016 08:43

javiereguiluz added this to the 3.2 milestone Nov 7, 2016

fabpot removed this from the 3.2 milestone Nov 16, 2016

nicolas-grekas added this to the 2.7 milestone Dec 6, 2016

javiereguiluz added the Status: Waiting feedback label Jan 23, 2017

fabpot closed this Oct 1, 2017

soullivaneuh deleted the twig-form-bootstrap-empty-label-align branch January 14, 2018 13:17

Uh oh!

Do not put any empty div if label is disabled #19247

Do not put any empty div if label is disabled #19247

Uh oh!

Conversation

soullivaneuh commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Application

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude commented Jun 30, 2016

Uh oh!

greg0ire commented Jun 30, 2016

Uh oh!

soullivaneuh commented Jun 30, 2016

Uh oh!

HeahDude commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude commented Jun 30, 2016

Uh oh!

soullivaneuh commented Jun 30, 2016

Uh oh!

lyrixx commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soullivaneuh commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soullivaneuh commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jongotlin commented Jul 1, 2016

Uh oh!

soullivaneuh commented Jul 1, 2016

Uh oh!

jongotlin commented Jul 1, 2016

Uh oh!

soullivaneuh commented Jul 1, 2016

Uh oh!

jongotlin commented Jul 1, 2016

Uh oh!

soullivaneuh commented Jul 1, 2016

Uh oh!

jongotlin commented Jul 1, 2016

Uh oh!

soullivaneuh commented Jun 30, 2016 •

edited

Loading

HeahDude Jun 30, 2016 •

edited

Loading

soullivaneuh commented Jun 30, 2016 •

edited

Loading

greg0ire Sep 15, 2016 •

edited

Loading

soullivaneuh commented Sep 15, 2016 •

edited

Loading