[Form] Add Bootstrap 4 style for field FileType #26502

zzenmate · 2018-03-12T22:47:49Z

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

juanmiguelbesada · 2018-03-14T10:19:47Z

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

@@ -257,6 +267,14 @@
    </{{ element|default('div') }}>
 {%- endblock form_row %}

+{% block file_row -%}
+    <{{ element|default('div') }} class="form-group custom-file">


According to docs and the provided example, form-group class is not needed.

juanmiguelbesada · 2018-03-14T10:20:31Z

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

+{% block file_row -%}
+    <{{ element|default('div') }} class="form-group custom-file">
+        {{- form_label(form) -}}
+        {{- form_widget(form) -}}


The label should be after widget

… with file_row.

zzenmate · 2018-03-14T20:37:49Z

I added the most basic block for file_label and file_widget for overriding form-control-label, form-control-file and using custom-file-input, custom-file-label.

juanmiguelbesada · 2018-03-14T20:44:18Z

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

@@ -182,7 +182,7 @@

 {% block file_widget -%}
    {%- set attr = attr|merge({class: (attr.class|default('') ~ ' custom-file-input')|trim}) -%}
-    {{- block('form_widget_simple') }}
+    {{ block("form_widget_simple", "form_div_layout.html.twig") }}


Double quotes

juanmiguelbesada · 2018-03-14T20:50:47Z

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

@@ -182,7 +182,7 @@

 {% block file_widget -%}
    {%- set attr = attr|merge({class: (attr.class|default('') ~ ' custom-file-input')|trim}) -%}
-    {{- block('form_widget_simple') }}
+    {{ block("form_widget_simple", "form_div_layout.html.twig") }}


why did you remove the "No leading/trailing whitespace"?
Also double quotes should be single quotes

juanmiguelbesada · 2018-03-14T20:51:25Z

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

@@ -252,7 +252,7 @@

 {% block file_label -%}
    {%- set label_attr = label_attr|merge({class: (label_attr.class|default('') ~ ' custom-file-label')|trim}) -%}
-    {{- block('form_label') -}}
+    {{ block("form_label", "form_div_layout.html.twig") }}


Same here, leading/trailing whitespace and double quotes

javiereguiluz

It looks good to me, but @Nyholm and @mpiot have more experience with this Bootstrap form theme and could review it too. Thanks!

mpiot · 2018-03-16T11:25:05Z

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

@@ -180,6 +180,11 @@
    </div>
 {%- endblock choice_widget_expanded %}

+{% block file_widget -%}


In the form_simple_widget there is a test on the type of form, if it's a file, it change the class. Adapt it is a better idea than do this block, no ?

{%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-control' ~ (type|default('') == 'file' ? '-file' : ''))|trim}) -%}

Do it:

{%- set attr = attr|merge({class: (attr.class|default('') ~ (type|default('') == 'file' ? 'custom-file-input' : 'form-control'))|trim}) -%}

mpiot · 2018-03-16T11:38:59Z

We try to put errors in the label to respect a standard for blind people, but I think it's difficult in this case.. Because the label is directly in the field. I look if there is a way to do it.

mpiot · 2018-03-16T11:56:28Z

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

@@ -257,6 +267,14 @@
    </{{ element|default('div') }}>
 {%- endblock form_row %}

+{% block file_row -%}
+    <{{ element|default('div') }} class="custom-file">


In your case the element is the div, maybe use
directly in place of element ? And you should add a <div class="input-group mb-3"> or a simple form-group or at least the mb-3 to add a margin after the row.

Doesn't custom-file-label add a margin-bottom? (.5rem to be exactly)

I've try and it's collapsed. In the bootstrap doc, they encapsulate the custom-file-label in an other div with a mb-3 to add the margin bottom, because they use input-group class to add many buttons. In our case, we can use the form-group that add the mb-3 (maybe colloration when fail ?)

You need to add a div with class "form-group" around this.

Nyholm

Great. I just tested this and it looks okey. I added a few comments.

Nyholm · 2018-03-18T13:39:06Z

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

@@ -257,6 +267,14 @@
    </{{ element|default('div') }}>
 {%- endblock form_row %}

+{% block file_row -%}
+    <{{ element|default('div') }} class="custom-file">


You need to add a div with class "form-group" around this.

Nyholm · 2018-03-18T13:39:52Z

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

@@ -180,6 +180,11 @@
    </div>
 {%- endblock choice_widget_expanded %}

+{% block file_widget -%}
+    {%- set attr = attr|merge({class: (attr.class|default('') ~ ' custom-file-input')|trim}) -%}
+    {{- block('form_widget_simple', 'form_div_layout.html.twig') -}}


You should remove 'form_div_layout.html.twig'. Or maybe Im missing something. Why did you add it here?

I use form_div_layout.html.twig from form_widget_simple for overriding form_widget_simple from bootstrap_4_layout.html.twig.
Because we don't need merge .form-control-file with .custom-file-input.
I think we will need use only one class. And it will be .custom-file-input.
In documention used only .custom-file-input
Or am I wrong?

Hm... Okey. Why are we adding form-control-file. Is that ever needed?

https://getbootstrap.com/docs/4.0/components/input-group/#custom-file-input
In this example, we also not used form-control-file or form-control-label.

Excuse my ignorance. But in what scenario/input do we need form-control-file?

This block can be removed if you apply the solution specified before: edit the form_simple_widget. Your solution remove the way to use the previous possibilities to display file input, the only way become the custom way.

Then, just edit the part where we set the .form-control-file in place of .form-control, I've write the code to replace on the top. You just need to edit a line in the block form_simple_widget and remove this block.

I regive you the line of code:

In the form_simple_widget block there is:

{%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-control' ~ (type|default('') == 'file' ? '-file' : ''))|trim}) -%}

Replace by this line:

{%- set attr = attr|merge({class: (attr.class|default('') ~ (type|default('') == 'file' ? 'custom-file-input' : 'form-control'))|trim}) -%}

Then, if the type is a file the attr become custom-file-input in place of form-control. The code is already ok, you just have to adapt it.

We need form-control-file for adding: "display: block; width: 100%;".
But, I am don't know if we need to use it in this case.

In the demo they write it:

<div class="input-group mb-3"> <div class="custom-file"> <input type="file" class="custom-file-input" id="inputGroupFile02"> <label class="custom-file-label" for="inputGroupFile02">Choose file</label> </div> </div>

The input just have the class custom-file-input, this is what do the line:

{%- set attr = attr|merge({class: (attr.class|default('') ~ (type|default('') == 'file' ? 'custom-file-input' : 'form-control'))|trim}) -%}

Nyholm · 2018-03-18T13:40:17Z

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

@@ -180,6 +180,11 @@
    </div>
 {%- endblock choice_widget_expanded %}

+{% block file_widget -%}


Nyholm · 2018-03-18T13:40:50Z

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

@@ -245,6 +250,11 @@
    {%- endif -%}
 {%- endblock checkbox_radio_label %}

+{% block file_label -%}
+    {%- set label_attr = label_attr|merge({class: (label_attr.class|default('') ~ ' custom-file-label')|trim}) -%}
+    {{- block('form_label', 'form_div_layout.html.twig') -}}


Do you use form_div_layout.html.twig to avoid errors in the label?

As I wrote on top, we don't need to merge 2 class: "custom-file-label" and "form-control-label".
Or am I wrong?

Im not sure. Ill check later tonight

In this case is it not better to replace (replace filter) an existing class in place of do a double override on the block ? Because if we extend a file, that extend a file, each file override a block, and finally we re-override the block again, it's more difficult to understand what we do, no ?

Yes, you're right. But, How to get input type in form_label for write logic with if.
I mean something like that:

{% if type == 'file' %} custom-file-label {% else %} form-control-label {% endif %}

Yes, in the block form_label, you can add this:

{% elseif type is defined and type == 'file' %} {%- set label_attr = label_attr|merge({for: id, class: (label_attr.class|default('') ~ ' custom-file-label')|trim}) -%}

To obtain:

{%- if compound is defined and compound -%} {%- set element = 'legend' -%} {%- set label_attr = label_attr|merge({class: (label_attr.class|default('') ~ ' col-form-label')|trim}) -%} {% elseif type is defined and type == 'file' %} {%- set label_attr = label_attr|merge({for: id, class: (label_attr.class|default('') ~ ' custom-file-label')|trim}) -%} {%- else -%} {%- set label_attr = label_attr|merge({for: id, class: (label_attr.class|default('') ~ ' form-control-label')|trim}) -%} {%- endif -%}

Then, you can remove your the block file_label. Keep the type is defined test, because it appears that it is not always defined.

Ha, and then you need to change something fo errors, because by using our form_label, you have the errors displayed in it.

I've try, and if you remove the error handling in your file_row, error are displayed semi-correctly... But I think it's ok.

In this case, you can both: use your file_row, or add a check on type == file in the form_row. I think it's more readable with your way, but we re-write code, for it, do as you wish :-) But you need to remove the form_error from the file_row since you use our form_label to avoid error duplication.

@mpiot Thank you for your help.

zzenmate · 2018-03-20T12:41:47Z

status: needs review

Nyholm · 2018-03-20T12:49:14Z

There are still some work. Please see #26502 (comment)

mpiot · 2018-03-20T20:24:05Z

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

@@ -180,6 +180,11 @@
    </div>
 {%- endblock choice_widget_expanded %}

+{% block file_widget -%}
+    {%- set attr = attr|merge({class: (attr.class|default('') ~ ' custom-file-input')|trim}) -%}
+    {{- block('form_widget_simple', 'form_div_layout.html.twig') -}}


This block can be removed if you apply the solution specified before: edit the form_simple_widget. Your solution remove the way to use the previous possibilities to display file input, the only way become the custom way.

Then, just edit the part where we set the .form-control-file in place of .form-control, I've write the code to replace on the top. You just need to edit a line in the block form_simple_widget and remove this block.

I regive you the line of code:

In the form_simple_widget block there is:

{%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-control' ~ (type|default('') == 'file' ? '-file' : ''))|trim}) -%}

Replace by this line:

{%- set attr = attr|merge({class: (attr.class|default('') ~ (type|default('') == 'file' ? 'custom-file-input' : 'form-control'))|trim}) -%}

Then, if the type is a file the attr become custom-file-input in place of form-control. The code is already ok, you just have to adapt it.

mpiot · 2018-03-20T20:35:23Z

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

@@ -245,6 +250,11 @@
    {%- endif -%}
 {%- endblock checkbox_radio_label %}

+{% block file_label -%}
+    {%- set label_attr = label_attr|merge({class: (label_attr.class|default('') ~ ' custom-file-label')|trim}) -%}
+    {{- block('form_label', 'form_div_layout.html.twig') -}}


In this case is it not better to replace (replace filter) an existing class in place of do a double override on the block ? Because if we extend a file, that extend a file, each file override a block, and finally we re-override the block again, it's more difficult to understand what we do, no ?

mpiot · 2018-03-20T20:37:30Z

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

@@ -257,6 +267,16 @@
    </{{ element|default('div') }}>
 {%- endblock form_row %}

+{% block file_row -%}
+    <{{ element|default('div') }} class="form-group">


In this case the element can't be another thing than a div element, maybe it's better to directly write div in place of use the element var ? It works like it, it's just a question/suggestion. ;-)

zzenmate · 2018-03-26T20:33:50Z

status: needs review

mpiot

It seems all is ok, thanks for your help :-)

Nyholm

Good. Just a super minor

Nyholm · 2018-03-26T20:51:28Z

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

@@ -117,7 +117,7 @@

 {% block form_widget_simple -%}
    {% if type is not defined or type != 'hidden' %}
-        {%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-control' ~ (type|default('') == 'file' ? '-file' : ''))|trim}) -%}
+        {%- set attr = attr|merge({class: (attr.class|default('') ~ (type|default('') == 'file' ? ' custom-file-input' : ' form-control'))|trim}) -%}


You need a space here.

“ file”

A space ? It's a test on the type of the form, why add a space before ? It can't match if we add it.

No, you are correct. It is the value that should have space and they do.

Nyholm

Thank you for this PR.

im 👍

Nyholm

Wrong button :/

Im 👍

fabpot · 2018-03-27T07:05:33Z

Thank you @zzenmate.

This PR was squashed before being merged into the 4.1-dev branch (closes #26502). Discussion ---------- [Form] Add Bootstrap 4 style for field FileType | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #26464  | License | MIT | Doc PR | symfony/symfony-docs#...   Commits ------- df57718 [Form] Add Bootstrap 4 style for field FileType

Add skeleton for file twig block

5e74525

carsonbot added Status: Needs Review Form Feature labels Mar 12, 2018

zzenmate mentioned this pull request Mar 12, 2018

[Form] Add Bootstrap 4 style for field FileType #26464

Closed

fix with div

4202ea6

zzenmate changed the title ~~[WIP] [Form] Add Bootstrap 4 style for field FileType~~ [Form] Add Bootstrap 4 style for field FileType Mar 13, 2018

carsonbot removed the Status: Needs Review label Mar 14, 2018

juanmiguelbesada suggested changes Mar 14, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Mar 14, 2018

nicolas-grekas added this to the 4.1 milestone Mar 14, 2018

Add use the most basic template for file_widget, file_label. Some fix…

b2522da

… with file_row.

juanmiguelbesada suggested changes Mar 14, 2018

View reviewed changes

Fix with leading/trailing whitespace and double quotes

8712a12

ghost approved these changes Mar 15, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Mar 15, 2018

juanmiguelbesada approved these changes Mar 15, 2018

View reviewed changes

javiereguiluz approved these changes Mar 16, 2018

View reviewed changes

carsonbot removed the Status: Reviewed label Mar 16, 2018

mpiot suggested changes Mar 16, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Mar 16, 2018

mpiot suggested changes Mar 16, 2018

View reviewed changes

Nyholm requested changes Mar 18, 2018

View reviewed changes

carsonbot added Status: Needs Review and removed Status: Needs Work labels Mar 20, 2018

Add form-group around custom-file.

6d920c3

carsonbot removed the Status: Needs Review label Mar 20, 2018

mpiot suggested changes Mar 20, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Mar 20, 2018

zenmate added 3 commits March 22, 2018 23:53

Improve template logic for file.

31ffffd

Improve template logic for file form label.

64340dc

remove file_label block

e54937e

carsonbot added Status: Needs Review and removed Status: Needs Work labels Mar 26, 2018

mpiot approved these changes Mar 26, 2018

View reviewed changes

carsonbot added Status: Reviewed Status: Needs Work and removed Status: Needs Review Status: Reviewed labels Mar 26, 2018

Nyholm requested changes Mar 26, 2018

View reviewed changes

carsonbot removed the Status: Needs Work label Mar 26, 2018

Nyholm approved these changes Mar 26, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Mar 26, 2018

Nyholm approved these changes Mar 26, 2018

View reviewed changes

fabpot closed this Mar 27, 2018

zzenmate deleted the bootstrap-file branch March 27, 2018 07:33

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

[Form] Add Bootstrap 4 style for field FileType #26502

[Form] Add Bootstrap 4 style for field FileType #26502

Conversation

zzenmate commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzenmate commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz left a comment

Choose a reason for hiding this comment

mpiot Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpiot commented Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpiot Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzenmate commented Mar 20, 2018

Nyholm commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzenmate commented Mar 26, 2018

mpiot left a comment

Choose a reason for hiding this comment

Nyholm left a 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

Nyholm left a comment

Choose a reason for hiding this comment

Nyholm left a comment

Choose a reason for hiding this comment

fabpot commented Mar 27, 2018

mpiot Mar 16, 2018 •

edited

Loading

mpiot commented Mar 16, 2018 •

edited

Loading

mpiot Mar 16, 2018 •

edited

Loading