Bootstrap4 support for Twig form theme #19648

hiddewie · 2016-08-17T10:22:18Z

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

I have made a port of the Twig form theming code for Bootstrap 3 to the α-5 version of Bootstrap 4.

The (inheritance) structure of the form theming files has changed because a number of blocks are the same between BS 3 and 4. They have been migrated to bootstrap_base_layout.html.twig.
The new tree is as follows:

bootstrap_base_layout.html.twig
|-- bootstrap_3_layout.html.twig
|   `-- bootstrap_3_horizontal_layout.html.twig
`-- bootstrap_4_layout.html.twig
    `-- bootstrap_4_horizontal_layout.html.twig

Any occurances of .form-horizontal have been removed from the BS 4 code.
Checkboxes and radio buttons have been stacked using the .form-check, .form-check-label and .form-check-input classes. There is now no distinction between checkboxes and radio buttons in the markdown.
All layout tests have been added and updated for BS4. The inheritance tree is as follows:

AbstractLayoutTest
`-- AbstractBootstrap3LayoutTest
    |-- AbstractBootstrap3HorizontalLayoutTest
    `-- AbstractBootstrap4LayoutTest
        `-- AbstractBootstrap4HorizontalLayoutTest

All tests pass. The classes FormExtensionBootstrap4LayoutTest and FormExtensionBootstrap4HorizontalLayoutTest have been created similarly to the BS 3 versions.

~~The label coloring on an validation is not correct. I've made an issue (Form validation labels not coloring twbs/bootstrap#20535) of the problem.~~
No custom form elements have been used.
A docs PR can be created if this PR is accepted.
The new code might have to be updated if large changes occur in the BS 4 α.

Screenshot of BS3 and 4 comparison for the same form:

HeahDude · 2016-08-17T13:04:56Z

There were already #16290, but at first sight this PR adds tests and it's a good point :) thanks for working on this!

hiddewie · 2016-08-17T13:17:10Z

Strange, I did not notice the link of the other PR in the original ticket. Thank you for notifying me.

hiddewie · 2016-08-17T13:23:54Z

I do not understand the fail of https://travis-ci.org/symfony/symfony/jobs/152929965 (class missing?).

Also, the PR #16290 does not seem up-to-date with the latest classes and HTML structure of BS 4.

HeahDude · 2016-08-17T13:34:38Z

I do not understand the fail of https://travis-ci.org/symfony/symfony/jobs/152929965 (class missing?).

the twig bridge now needs the form component to be >= 3.2, so you need to update its composer.json accordingly.

Simperfit · 2016-08-18T17:49:37Z

You should consider adding an author tag on the tests class, it's alot of work and we should know who was behind this ;)

hiddewie · 2016-09-18T10:02:46Z

What is the review status of this PR?

More and more use cases pop up as the Bootstrap 4 alpha becomes more stable and the Bootstrap 3 feature development had been ceased.

fabpot · 2016-09-18T14:50:00Z

I would be more than happy to merge this PR before 3.2 end of dev (which happens by the end of the month).

fabpot · 2016-09-18T15:05:23Z

At least, this PR needs to be rebased to get rid of the merge commit.

hiddewie · 2016-09-18T17:19:06Z

Rebase done.

fabpot · 2016-09-18T23:05:06Z

Any Bootstrap 4 users here to help us review this pull request?

hiddewie · 2016-09-22T14:39:17Z

Maybe @lyrixx as original creator of the Bootstrap 3 files?

lyrixx · 2016-09-22T15:24:54Z

Sorry, I never used bootstrap 4.

mahono · 2016-10-05T12:21:49Z

I'll have a look at it this week for a new project and will be able to give feedback by the end of the week. Hope it's not too late.

eschmar · 2016-10-05T15:17:08Z

One thing that i've noticed, is that for expanded fields it would be nice to use the fieldset tag with a nested legend instead of a label. It's the correct way and functionality like disabling all controls within a fieldset will be enabled.

Maybe I can have a look on the weekend, but if anyone else can, please go for it!

hiddewie · 2016-10-05T21:20:41Z

@eschmar I think the idea of adding a fieldset with a legend to an expanded field is nice. I will look into that.

However, the browser support for adding disabled is in my opinion not good enough (practically no IE support) to make us of it.

mahono · 2016-10-07T16:38:10Z

I created some forms today using these templates. It looks pretty good to me. Works nice with current Bootstrap 4. I also like the idea of separating templates into an inheritance structure very much. To be honest I'm not enough Bootstrap 4 expert to say that everything works perfect.

eschmar · 2016-10-10T07:04:53Z

Thanks @hiddewie for all your efforts! I agree regarding the disabled state.

I just migrated the forms of a smaller project and it works great! The fieldset tag could be improved more. Now it's rendered correctly for radio boxes, but for example checkboxes get a label tag with css class .col-form-legend. This should also be a legend.

One step even further, would be the possibility to enable the fieldset tag for entire other form types. For example, I have an AddressType. Bootstrap has support for <fieldset class="form-group">.

Great work!

hiddewie · 2016-10-10T19:39:28Z

@eschmar Could you post your form type with checkboxes? This should be fixed, but I cannot find your problem (any checkboxes I can try render a <fieldset> with <legend> correctly).

eschmar · 2016-10-11T11:43:19Z

@hiddewie Excuse me, it was an expanded EntityType:

hiddewie · 2016-10-13T14:17:06Z

@eschmar

Maybe I am missing something. My entity type is also rendering correctly as fieldset + legend:

Type:

HTML:

Horizontal layout:

stof · 2016-10-13T14:29:19Z

@eschmar make sure that you don't have your own form theme overriding this partially

stof · 2016-10-13T14:30:24Z

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

+{# Labels #}
+
+{% block form_label -%}
+{% spaceless %}


please use whitespace control, not spaceless. Spaceless has a huge performance impact as it processing the template output with a regex and form rendering are using these blocks a lot

stof · 2016-10-13T14:31:18Z

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

+
+{% block form_row -%}
+    {%- if expanded is defined and expanded %}
+        {% set element = 'fieldset' %}


some whitespace control is missing here to ensure we don't have remaining spaces

stof · 2016-10-13T14:31:35Z

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

+            {{- form_widget(form) -}}
+            {{- form_errors(form) -}}
+        </div>
+{##}</{{ element|default('div') }} >


you have a useless space after the }}

stof · 2016-10-13T14:31:52Z

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

+    {%- if expanded is defined and expanded %}
+        {% set element = 'fieldset' %}
+    {%- endif -%}
+    <{{ element|default('div') }}  class="form-group row{% if (not compound or force_error|default(false)) and not valid %} has-danger{% endif %}">


you have 2 spaces before class while 1 is enough

stof · 2016-10-13T14:32:21Z

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

+{%- endblock checkbox_radio_row %}
+
+{% block submit_row -%}
+{% spaceless %}


please don't use spaceless

stof · 2016-10-13T14:32:53Z

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

+        {% if required %}
+            {% set label_attr = label_attr|merge({class: (label_attr.class|default('') ~ ' required')|trim}) %}
+        {% endif %}
+        {% if parent_label_class is defined %}


a bnch of whitespace-control markers are missing here

stof · 2016-10-13T14:33:26Z

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

+            <li>{{ error.message }}</li>
+        {%- endfor -%}
+    </ul>
+    {% if form.parent %}</div>{% else %}</div>{% endif %}


why using a if to display the same in both cases ?

stof · 2016-10-13T14:33:55Z

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

+
+{% block form_errors -%}
+    {% if errors|length > 0 -%}
+    {% if form.parent %}<div class="form-control-feedback">{% else %}<div class="alert alert-danger">{% endif %}


I would use the if only around classes rather than duplicating the whole div

stof · 2016-10-13T14:34:19Z

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

+{% block money_widget -%}
+    <div class="input-group">
+        {% set append = money_pattern starts with '{{' %}
+        {% if not append %}


missing whitespace controls here

hiddewie · 2016-10-14T15:45:03Z

@stof Thank you. Done. Some of the whitespace problems were already in the existing code. They have been fixed anyway.

eschmar

@hiddewie Sorry for the confusion. There was an issue with how i embedded the form themes. Everything is rendering correctly now!

I tried to find an easy way to mark custom form types to use the fieldset tag, however couldn't find one which didn't involve a lot of changes. An example: I have a custom AddressType, where the expanded option can not be set. But i guess that's a bit out of scope.

This PR is ready for merge, in my opinion.

eschmar · 2016-10-17T10:43:25Z

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

+            {{- form_widget(form) -}}
+            {{- form_errors(form) -}}
+        </div>
+{##}</{{ element|default('div') }}>


Is there a reason for this (empty) comment?

The comment was in the previous code (https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig#L34). It was probably put there to avoid whitespace issues (the commit I can find is a35d3d4).

hiddewie · 2016-10-17T11:16:55Z

@eschmar Thank you.

I think your request is a valid feature, but should be put into a new issue because (as you mention) it goes well out of scope of this PR/changeset.

hiddewie · 2016-11-06T17:34:22Z

Ping @fabpot. Is there any progress on this feature being in the 3.2 release, or is it being pushed forward to 3.3?

fabpot · 2016-11-06T17:39:49Z

I would be for 3.3 now. Apart from feedback from others, anything else to be done before merging?

hiddewie · 2016-11-06T17:44:28Z

I've tried to fix all comments up to now, both code style and code content.

If there are more new or existing comments I have missed, then I will of course process them.

alex-bacart · 2016-12-22T15:33:46Z

@hiddewie margin and padding styles have been changed in Bootstrap v4.0.0-alpha.5 from m-*-* to m*-* and p-*-* to p*-* (see https://blog.getbootstrap.com/2016/10/19/bootstrap-4-alpha-5/#utilities-overhaul)

Error list <ul class="list-unstyled m-b-0"> is old-styled in bootstrap_4_layout.html.twig.

hiddewie · 2016-12-23T10:39:54Z

@bacart Thanks for noticing me, I've updated the Twig code.

havvg · 2016-12-29T14:01:35Z

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

+    {%- if expanded is defined and expanded -%}
+        {%- set element = 'fieldset' -%}
+    {%- endif -%}
+    <{{ element|default('div') }} class="form-group row{% if (not compound or force_error|default(false)) and not valid %} has-danger{% endif %}">


You should not add the grid classes (e.g.) row within the form theme. This is mixing concerns of the respective CSS classes. If you would like to make a certain form-group a row, too, you should do this using the frontend compilation (e.g. SASS mixins) and apply proper changes to the addressed form / form group / ...

See http://v4-alpha.getbootstrap.com/components/forms/#using-the-grid. How would you suggest we support horizontal forms in this form theme if not using the default form-group & row combination which is required in BS 4?

I'm sorry, I totally missed this being the horizontal one, I was on the path it's the base one.

No problem! Thanks for the review.

alex-bacart · 2017-01-11T14:39:47Z

@hiddewie one more notice:
{% block button_widget -%} and {% block choice_widget_collapsed -%} are not properly closed.
{%- endblock %} should be {%- endblock button_widget %} and {%- endblock choice_widget_collapsed %} respectively.

UPD: typo came from bootstrap_3_layout.html.twig.

hiddewie · 2017-01-11T16:02:10Z

@bacart This has been fixed. Although in the older code not all blocks were closed with {% endblock <name> %} either (see for example https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/foundation_5_layout.html.twig#L20).

hiddewie · 2017-02-21T19:48:16Z

I don't get why GitHub closed my PR. It seems it does not recognise my force push of a rebase onto master...

However, the compare is not empty when I do it manually: master...hiddewie:bootstrap4

…ereguiluz) This PR was merged into the 3.4 branch. Discussion ---------- Bootstrap4 support for Twig form theme **This PR is a followup from #19648. That PR was closed because GitHub thought my branch contained no commits after a force push...** | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #16289 | | License | MIT | | Doc PR | - | I have made a port of the Twig form theming code for Bootstrap 3 to the α-5 version of Bootstrap 4. - The (inheritance) structure of the form theming files has changed because a number of blocks are the same between BS 3 and 4. They have been migrated to `bootstrap_base_layout.html.twig`. The new tree is as follows: ``` bootstrap_base_layout.html.twig |-- bootstrap_3_layout.html.twig | `-- bootstrap_3_horizontal_layout.html.twig `-- bootstrap_4_layout.html.twig `-- bootstrap_4_horizontal_layout.html.twig ``` - Any occurances of `.form-horizontal` have been removed from the BS 4 code. - Checkboxes and radio buttons have been stacked using the `.form-check`, `.form-check-label` and `.form-check-input` classes. There is now no distinction between checkboxes and radio buttons in the markdown. - All layout tests have been added and updated for BS4. The inheritance tree is as follows: ``` AbstractLayoutTest `-- AbstractBootstrap3LayoutTest |-- AbstractBootstrap3HorizontalLayoutTest `-- AbstractBootstrap4LayoutTest `-- AbstractBootstrap4HorizontalLayoutTest ``` All tests pass. The classes `FormExtensionBootstrap4LayoutTest` and `FormExtensionBootstrap4HorizontalLayoutTest` have been created similarly to the BS 3 versions. - ~~The label coloring on an validation is not correct. I've made an issue (twbs/bootstrap#20535) of the problem.~~ - No [custom form elements](http://v4-alpha.getbootstrap.com/components/forms/#custom-forms) have been used. - A docs PR can be created if this PR is accepted. - The new code might have to be updated if large changes occur in the BS 4 α. Screenshot of BS3 and 4 comparison for the same form: ![1](https://cloud.githubusercontent.com/assets/1073881/17732594/dfcb50d6-6472-11e6-8e96-c46987809322.PNG) Commits ------- f12e588 Removed unneeded wrapping quotes around a Twig key 709f134 Removed unneeded wrapping quotes around a Twig key 4222d54 Initial commit for Bootstrap 4 form theme, based on Bootstrap 3 form theme

carsonbot added Status: Needs Review Feature labels Aug 17, 2016

stof requested changes Oct 13, 2016

View reviewed changes

eschmar reviewed Oct 17, 2016

View reviewed changes

javiereguiluz added this to the 3.3 milestone Nov 6, 2016

javiereguiluz mentioned this pull request Dec 22, 2016

[WIP] [Form] Added support for Bootstrap 4 forms #16290

Closed

havvg reviewed Dec 29, 2016

View reviewed changes

hiddewie closed this Feb 21, 2017

hiddewie mentioned this pull request Feb 24, 2017

Bootstrap4 support for Twig form theme #21751

Merged

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

Bootstrap4 support for Twig form theme #19648

Bootstrap4 support for Twig form theme #19648

Conversation

hiddewie commented Aug 17, 2016 • edited Loading

HeahDude commented Aug 17, 2016

hiddewie commented Aug 17, 2016 • edited Loading

hiddewie commented Aug 17, 2016

HeahDude commented Aug 17, 2016

Simperfit commented Aug 18, 2016

hiddewie commented Sep 18, 2016

fabpot commented Sep 18, 2016

fabpot commented Sep 18, 2016

hiddewie commented Sep 18, 2016

fabpot commented Sep 18, 2016

hiddewie commented Sep 22, 2016

lyrixx commented Sep 22, 2016

mahono commented Oct 5, 2016

eschmar commented Oct 5, 2016

hiddewie commented Oct 5, 2016

mahono commented Oct 7, 2016

eschmar commented Oct 10, 2016

hiddewie commented Oct 10, 2016

eschmar commented Oct 11, 2016

hiddewie commented Oct 13, 2016

stof commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddewie commented Oct 14, 2016

eschmar left a comment

Choose a reason for hiding this comment

eschmar Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddewie commented Oct 17, 2016

hiddewie commented Nov 6, 2016

fabpot commented Nov 6, 2016

hiddewie commented Nov 6, 2016

alex-bacart commented Dec 22, 2016

hiddewie commented Dec 23, 2016

havvg Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havvg Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-bacart commented Jan 11, 2017 • edited Loading

hiddewie commented Jan 11, 2017

hiddewie commented Feb 21, 2017

hiddewie commented Aug 17, 2016 •

edited

Loading

hiddewie commented Aug 17, 2016 •

edited

Loading

eschmar Oct 17, 2016 •

edited

Loading

havvg Dec 29, 2016 •

edited

Loading

havvg Jan 2, 2017 •

edited

Loading

alex-bacart commented Jan 11, 2017 •

edited

Loading