Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Add Bootstrap 4 style for field FileType#26502
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
zzenmate commentedMar 12, 2018
| 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#... |
| {%-endblockform_row %} | ||
| {%blockfile_row -%} | ||
| <{{element|default('div') }} class="form-group custom-file"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
According todocs and the provided example, form-group class is not needed.
| {%blockfile_row -%} | ||
| <{{element|default('div') }} class="form-group custom-file"> | ||
| {{- form_label(form) -}} | ||
| {{- form_widget(form) -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The label should be after widget
zzenmate commentedMar 14, 2018
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. |
| {%blockfile_widget -%} | ||
| {%-setattr=attr|merge({class: (attr.class|default('') ~' custom-file-input')|trim}) -%} | ||
| {{-block('form_widget_simple') }} | ||
| {{block("form_widget_simple","form_div_layout.html.twig") }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Double quotes
| {%blockfile_widget -%} | ||
| {%-setattr=attr|merge({class: (attr.class|default('') ~' custom-file-input')|trim}) -%} | ||
| {{-block('form_widget_simple') }} | ||
| {{block("form_widget_simple","form_div_layout.html.twig") }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why did you remove the "No leading/trailing whitespace"?
Also double quotes should be single quotes
| {%blockfile_label -%} | ||
| {%-setlabel_attr=label_attr|merge({class: (label_attr.class|default('') ~' custom-file-label')|trim}) -%} | ||
| {{-block('form_label') -}} | ||
| {{block("form_label","form_div_layout.html.twig")}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here, leading/trailing whitespace and double quotes
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| </div> | ||
| {%-endblockchoice_widget_expanded %} | ||
| {%blockfile_widget -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In theform_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}) -%}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
mpiot commentedMar 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
| {%-endblockform_row %} | ||
| {%blockfile_row -%} | ||
| <{{element|default('div') }} class="custom-file"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In your case the element is the div, maybe use
<div> or a simpleform-group or at least themb-3 to add a margin after the row.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Doesn't custom-file-label add a margin-bottom? (.5rem to be exactly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You need to add adiv with class "form-group" around this.
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great. I just tested this and it looks okey. I added a few comments.
| {%-endblockform_row %} | ||
| {%blockfile_row -%} | ||
| <{{element|default('div') }} class="custom-file"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You need to add adiv with class "form-group" around this.
| {%blockfile_widget -%} | ||
| {%-setattr=attr|merge({class: (attr.class|default('') ~' custom-file-input')|trim}) -%} | ||
| {{-block('form_widget_simple','form_div_layout.html.twig') -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should remove 'form_div_layout.html.twig'. Or maybe Im missing something. Why did you add it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Indocumention used only .custom-file-input
Or am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hm... Okey. Why are we addingform-control-file. Is that ever needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Excuse my ignorance. But in what scenario/input do we need form-control-file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This block can be removed if you apply the solution specified before: edit theform_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 blockform_simple_widget and remove this block.
I regive you the line of code:
In theform_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 theattr becomecustom-file-input in place ofform-control. The code is already ok, you just have to adapt it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the demo they write it:
<div> <div> <input type="file"> <label for="inputGroupFile02">Choose file</label> </div></div>The input just have the classcustom-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}) -%}| </div> | ||
| {%-endblockchoice_widget_expanded %} | ||
| {%blockfile_widget -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| {%blockfile_label -%} | ||
| {%-setlabel_attr=label_attr|merge({class: (label_attr.class|default('') ~' custom-file-label')|trim}) -%} | ||
| {{-block('form_label','form_div_layout.html.twig') -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you useform_div_layout.html.twig to avoid errors in the label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As I wrote on top, we don't need to merge 2 class: "custom-file-label" and "form-control-label".
Or am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Im not sure. Ill check later tonight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 %}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, in theblock 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 blockfile_label. Keep thetype is defined test, because it appears that it is not always defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ha, and then you need to change something fo errors, because by using ourform_label, you have the errors displayed in it.
I've try, and if you remove the error handling in yourfile_row, error are displayed semi-correctly... But I think it's ok.
In this case, you can both: use yourfile_row, or add a check ontype == file in theform_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 theform_error from thefile_row since you use ourform_label to avoid error duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mpiot Thank you for your help.
zzenmate commentedMar 20, 2018
status: needs review |
Nyholm commentedMar 20, 2018
There are still some work. Please see#26502 (comment) |
| {%blockfile_widget -%} | ||
| {%-setattr=attr|merge({class: (attr.class|default('') ~' custom-file-input')|trim}) -%} | ||
| {{-block('form_widget_simple','form_div_layout.html.twig') -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This block can be removed if you apply the solution specified before: edit theform_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 blockform_simple_widget and remove this block.
I regive you the line of code:
In theform_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 theattr becomecustom-file-input in place ofform-control. The code is already ok, you just have to adapt it.
| {%blockfile_label -%} | ||
| {%-setlabel_attr=label_attr|merge({class: (label_attr.class|default('') ~' custom-file-label')|trim}) -%} | ||
| {{-block('form_label','form_div_layout.html.twig') -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ?
| {%-endblockform_row %} | ||
| {%blockfile_row -%} | ||
| <{{element|default('div') }} class="form-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In this case the element can't be another thing than adiv element, maybe it's better to directly writediv in place of use theelement var ? It works like it, it's just a question/suggestion. ;-)
zzenmate commentedMar 26, 2018
status: needs review |
mpiot left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It seems all is ok, thanks for your help :-)
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good. Just a super minor
| {%blockform_widget_simple -%} | ||
| {%iftypeis notdefinedortype!='hidden' %} | ||
| {%-setattr=attr|merge({class: (attr.class|default('') ~' form-control' ~(type|default('') =='file' ?'-file' :''))|trim}) -%} | ||
| {%-setattr=attr|merge({class: (attr.class|default('') ~ (type|default('') =='file' ?' custom-file-input' :' form-control'))|trim}) -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You need a space here.
“ file”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A space ? It's a test on thetype of the form, why add a space before ? It can't match if we add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, you are correct. It is the value that should have space and they do.
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for this PR.
im 👍
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wrong button :/
Im 👍
fabpot commentedMar 27, 2018
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<!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#26464 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features --><!--Write a short README entry for your feature/bugfix here (replace this comment block.)This will help people understand your PR and can be used as a start of the Doc PR.Additionally: - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch.-->Commits-------df57718 [Form] Add Bootstrap 4 style for field FileType