[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless #24728

arkste · 2017-10-28T20:57:20Z

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

As discussed in #24711 i reverted the change i did in bootstrap_3_layout.html.twig (which caused an unnecessary empty div-container in the vertical-layout), added the checkbox_row block to the bootstrap_3_horizontal_layout.html.twig and removed spaceless (as proposed in #24727).

since i added {#--#} in bootstrap 3, i did the same for the same horizontal blocks in bootstrap 4 as well.

I moved the form_label_class & form_group_class blocks to the top of bootstrap_3_horizontal_layout.html.twig & bootstrap_4_horizontal_layout.html.twig, this should improve DX as they were spreaded across the file.

#24702 affected the bootstrap 4 horizontal layout as well, so i added the checkbox_row block to bootstrap 4 too.

ping @fabpot @nicolas-grekas

arkste · 2017-10-30T12:13:11Z

I've added tests to make sure the checkbox row doesn't break again in bootstrap 3 & 4.

TwigBridge tests were failing because of the changes made in #24703, so i fixed them as well.

Tests are still failing, but this is related due to the timezone changes.

Please let me know if there's anything else i can do to get this merged.

stof · 2017-10-30T13:38:14Z

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

@@ -8,20 +16,14 @@
 {# Labels #}

 {% block form_label -%}
-{% spaceless %}
    {% if label is same as(false) %}
        <div class="{{ block('form_label_class') }}"></div>
    {% else %}


these tags are missing the whitespace control to remove indentation spaces

changed. please review the recent commit, thanks!

arkste · 2017-10-30T14:07:02Z

Status: Needs Review

arkste · 2017-10-30T14:35:00Z

@stof i've added more whitespace control tags to remove indentation spaces.

nicolas-grekas · 2017-10-30T15:35:40Z

deps=low tests should not fail: Form's lowest dep for the bridge should be bumped (or vice-versa, didn't look precisely)

arkste · 2017-10-30T18:14:10Z

@nicolas-grekas not sure what to do now, the deps are already on ~3.4|~4.0.

nicolas-grekas · 2017-10-30T18:17:39Z

should be ~3.4-beta2|~4.0

arkste · 2017-10-30T18:23:52Z

correct me if i'm wrong, but even changing the deps wont make the tests running green on deps=low, since travis runs the bridge tests from the vendor-folder (like /home/travis/build/symfony/symfony/src/Symfony/Bridge/Twig/vendor/symfony/form/Tests/AbstractLayoutTest.php:84) and the fixed tests are only in this PR.

nicolas-grekas · 2017-10-30T18:24:20Z

it will: the CI builds per PR packages for that situation

fabpot · 2017-10-30T18:31:41Z

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

@@ -1,5 +1,13 @@
 {% use "bootstrap_3_layout.html.twig" %}

+{% block form_label_class -%}
+col-sm-2
+{%- endblock form_label_class %}


Can you move back this snippet to where it was before? That would ease merging older branches. Thanks.

fabpot · 2017-10-30T18:32:15Z

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

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


Same for this one. We try to not move snippets of code around as it makes merging branches more difficult.

arkste · 2017-10-30T18:41:23Z

@fabpot i reverted the change for bootstrap 3 & 4.

fabpot · 2017-10-30T18:42:21Z

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

    {{- parent() -}}
 {%- endblock button_widget %}

 {% block checkbox_widget -%}
    {%- set parent_label_class = parent_label_class|default(label_attr.class|default('')) -%}
-    {% if 'checkbox-inline' in parent_label_class %}
+    {%- if 'checkbox-inline' in parent_label_class -%}


Are you sure those added - are needed? Does they make a difference as there are already - signs at the end of the line before and at the beginning of the next line?

actually they're not needed, but i tried to keep it similar to the bs4 layout, will remove them in both templates.

nicolas-grekas

the diff should be reduced to the strict minimum (there are some added "-" that are not needed)

nicolas-grekas · 2017-10-30T18:53:58Z

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

+            {{- form_errors(form) -}}
+        </div>{#--#}
+    </div>
+{%- endblock checkbox_row %}


missing end of line

fabpot · 2017-10-30T19:07:52Z

Thank you @arkste.

@fabpot

…y & remove spaceless (arkste) This PR was squashed before being merged into the 3.4 branch (closes #24728). Discussion ---------- [Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24711 | License | MIT | Doc PR | - As discussed in #24711 i reverted the change i did in `bootstrap_3_layout.html.twig` (which caused an unnecessary empty div-container in the vertical-layout), added the `checkbox_row` block to the `bootstrap_3_horizontal_layout.html.twig` and removed `spaceless` (as proposed in #24727). since i added `{#--#}` in bootstrap 3, i did the same for the same horizontal blocks in bootstrap 4 as well. I moved the `form_label_class` & `form_group_class` blocks to the top of `bootstrap_3_horizontal_layout.html.twig` & `bootstrap_4_horizontal_layout.html.twig`, this should improve DX as they were spreaded across the file. #24702 affected the bootstrap 4 horizontal layout as well, so i added the `checkbox_row` block to bootstrap 4 too. ping @fabpot @nicolas-grekas Commits ------- f84749f [Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless

arkste · 2017-10-30T19:11:47Z

seems like i pushed the EOL's while this was supposed to be merged. anything i should do now @fabpot ?

fabpot · 2017-10-30T19:12:43Z

@arkste all is fine :) Thank you.

arkste · 2017-10-30T19:13:31Z

ok :)

carsonbot added Status: Needs Review TwigBridge Bug labels Oct 28, 2017

arkste changed the base branch from master to 3.4 October 28, 2017 21:18

arkste added 3 commits October 30, 2017 12:59

[TwigBridge] fix bootstrap 3 & 4 checkbow row rendering

60105bc

[TwigBridge] Add Bootstrap 3 & 4 checkbow row tests

1fd87b0

[TwigBridge] fix bootstrap 4 tests for #24703

b5cdb92

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 30, 2017

stof requested changes Oct 30, 2017

View reviewed changes

[TwigBridge] add whitespace control to form_label

b370fe9

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 30, 2017

[TwigBridge] add whitespace control

0e7e4b6

arkste mentioned this pull request Oct 30, 2017

[TwigBridge] Bootstrap 4 form theme fixes #24703

Closed

nicolas-grekas changed the title ~~[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove …~~ [Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless Oct 30, 2017

[TwigBridge] bump dep on symfony/form

6dbdce0

nicolas-grekas added this to the 3.4 milestone Oct 30, 2017

fabpot requested changes Oct 30, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 30, 2017

fabpot reviewed Oct 30, 2017

View reviewed changes

arkste added 2 commits October 30, 2017 19:39

[TwigBridge] revert form_label_class & form_group_class change

9a0e8cb

[TwigBridge] revert form_label_class & form_group_class change

fe4eda6

fabpot reviewed Oct 30, 2017

View reviewed changes

arkste added 2 commits October 30, 2017 19:48

[TwigBridge] remove unnecessary whitespace control

f0e97ba

[TwigBridge] remove unnecessary whitespace control

6f07a0f

nicolas-grekas reviewed Oct 30, 2017

View reviewed changes

[TwigBridge] EOL added

cbecd02

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 30, 2017

fabpot approved these changes Oct 30, 2017

View reviewed changes

arkste closed this Oct 30, 2017

arkste deleted the spaceless-bootstrap-fix branch October 30, 2017 19:15

This was referenced Oct 30, 2017

Release v3.4.0-BETA2 #24761

Merged

Release v4.0.0-BETA2 #24762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless #24728

[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless #24728

arkste commented Oct 28, 2017 •

edited by nicolas-grekas

Loading

arkste commented Oct 30, 2017

stof Oct 30, 2017

arkste Oct 30, 2017

arkste commented Oct 30, 2017

arkste commented Oct 30, 2017

nicolas-grekas commented Oct 30, 2017

arkste commented Oct 30, 2017

nicolas-grekas commented Oct 30, 2017

arkste commented Oct 30, 2017

nicolas-grekas commented Oct 30, 2017

fabpot Oct 30, 2017

fabpot Oct 30, 2017

arkste commented Oct 30, 2017

fabpot Oct 30, 2017

arkste Oct 30, 2017

nicolas-grekas left a comment

nicolas-grekas Oct 30, 2017

fabpot commented Oct 30, 2017

arkste commented Oct 30, 2017

fabpot commented Oct 30, 2017

arkste commented Oct 30, 2017

[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless #24728

[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless #24728

Conversation

arkste commented Oct 28, 2017 • edited by nicolas-grekas Loading

arkste commented Oct 30, 2017

stof Oct 30, 2017

Choose a reason for hiding this comment

arkste Oct 30, 2017

Choose a reason for hiding this comment

arkste commented Oct 30, 2017

arkste commented Oct 30, 2017

nicolas-grekas commented Oct 30, 2017

arkste commented Oct 30, 2017

nicolas-grekas commented Oct 30, 2017

arkste commented Oct 30, 2017

nicolas-grekas commented Oct 30, 2017

fabpot Oct 30, 2017

Choose a reason for hiding this comment

fabpot Oct 30, 2017

Choose a reason for hiding this comment

arkste commented Oct 30, 2017

fabpot Oct 30, 2017

Choose a reason for hiding this comment

arkste Oct 30, 2017

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Oct 30, 2017

Choose a reason for hiding this comment

fabpot commented Oct 30, 2017

arkste commented Oct 30, 2017

fabpot commented Oct 30, 2017

arkste commented Oct 30, 2017

arkste commented Oct 28, 2017 •

edited by nicolas-grekas

Loading