-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Removed custom classes for input="file" types #27478
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@Nyholm that's for you :) |
@@ -116,7 +116,7 @@ | |||
|
|||
{% block form_widget_simple -%} | |||
{% if type is not defined or type != 'hidden' %} | |||
{%- set attr = attr|merge({class: (attr.class|default('') ~ (type|default('') == 'file' ? ' custom-file-input' : ' form-control'))|trim}) -%} | |||
{%- set attr = attr|merge({class: (attr.class|default('') ~ (type|default('') == 'file' ? ' ' : ' 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.
should be class "form-control-file" here, not empty
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 are right - Updated
@Nyholm Can you have a look 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.
Thank you for this PR.
Could you maybe explain why this is needed?
According to bootstrap documentation for file uploads, we are adding the correct classes.
From what I see it, you want to "opt-in" to boostrap file uploads instead of having them as default, is that correct?
After this PR is merged:
Im currently 👎
@@ -187,7 +187,7 @@ | |||
{%- 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}) -%} | |||
{%- set label_attr = label_attr|merge({for: id, class: (label_attr.class|default('') ~ ' ')|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.
Could be simplified to
{%- set label_attr = label_attr|merge({for: id, class: (label_attr.class|default(''))|trim}) -%}
@Nyholm I totally agree, the "design" is worse - but the UX (try selecting a file...) is much better. Also, the bootstrap documentation also have the "normal unstyled" as an example And if you want to have the selected file name in your file input, you will need to do something like this
The best thing would actually be, if it was possible to opt-out (or opt-in) somehow to either set the custom or the normal CSS classes. And then if you remove the label for the file input, which I think is quite normal if you are using multiple file uploads, then the whole file input is gone (even if you set a placeholder) |
I try to leave "design" and UX to bootstrap and just follow their recommendation. But I do recognize that boostrap is not providing the best experience when using multiple file inputs.
I agree, that would be the best. I would prefer if we could opt-out to not change the current behavior. How would one do this properly? By just adding a css class? |
Something like this?
Then in the form theme
Another option, could be directly on the |
Or maybe a form theme called "bootstrap_4_layout_nocustom.html.twig,
|
Interesting. But if you configure it in yaml, then there is no why to opt-in/out on case by case basis, right? |
Using "bootstrap_4_layout_nocustom.html.twig“ might be simpler. What if you just added a "bootstrap_4_layout_nocustom_file.html.twig” (not extending anything)? |
I agree on the nocustom template will be the best approach for opt-in/opt-out. Let me give it a try, should I create a new PR or do you want it in this, but with changed back the changes I made? |
Let’s keep the changes in this PR since we are trying to solve the same issue. |
@Nyholm does this mean you're +1? |
No, We are waiting for Martin to give a different implementation to fix this issue. |
Any news on this PR? @lsv Can you finish it or shall we close? |
Im sorry, I totally forgot about this in my holiday, I will have a commit up in within the next couple of days |
With this commit it will look like this Instead of How to use it change twig form theme from
To
I have also removed custom check/radio boxes, though its not required as the custom styled works properly. Also the
I have not changed it, but my nocustom changes the label to before the widget. |
I will close this PR, and make a new, I can see that symfony/master has changed a lot, and if I merge master, there will be a LOT of changed files in the commit. |
Before removing these classes a form with
input="file"
looked like thisNow after removing these