Skip to content

Allow user to set input class attributes #22622

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

Closed
wants to merge 1 commit into from
Closed

Conversation

gnat42
Copy link
Contributor

@gnat42 gnat42 commented May 3, 2017

When a user sets a form input attribute class, we shouldn't force-add the
form-control class.

Q A
Branch? 2.7
Bug fix? yes/no
New feature? no
BC breaks? maybe?
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

We are using a theme built on bootstrap 3. However we have cases where the input
class should be simply =col-xs-X for width control. When we set this on the form
we still get form-control and so the input is wide and ignores the other class.

It took awhile to find this where everything gets the form-control regardless of
having a class already set. It seems to me that if you are setting the class you
should set all classes required for what you are doing. This is a proof of concept
change at the moment as I didn't touch the textarea/buttons or any other widget
that uses the same syntax. Making this change locally keeps all other input fields
properly styled and allows us to override the class attribute as I would expect.
Our only other choice is to override the bootstrap_3_layout.html.twig if this
type of change is unwanted. If it is wanted, then I can make the rest of the changes.

When a user sets a form input attribute class, we shouldn't force-add the
form-control class.
@ro0NL
Copy link
Contributor

ro0NL commented May 3, 2017

This will break stuff.. yes :)

For being a bootstrap theme it makes sense to add the proper classes by definition, otherwise it wouldnt be a valid bootstrap theme. Meaning you need another extension point ;-) Perhaps refactor to something like #21751 so that classes are put in a separate block, which you can override.

@gnat42
Copy link
Contributor Author

gnat42 commented May 3, 2017

That still seems rather bizarre to me. Here's the basic issue:

Say I have

class MyFormType extends AbstractType 
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder->add('something',null, ['attr'=>['class'=>'some classes']]);
    }
}

I don't get an input with I get class="some classes form-control". I cannot in my form set the class. I agree that by default, the class should be added, but I disagree that it should be added to any input that has any class set, since as a dev I know what should be there.

I do realize that this will break others, but it really seems very bizarre to override a form theme so that my classes are applied.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone May 4, 2017
@xabbuh
Copy link
Member

xabbuh commented May 12, 2017

I don't get an input with I get class="some classes form-control". I cannot in my form set the class.

I don't understand this. Your class will be set anyway. It's just the form-control class is present too. So what's the issue with it?

Merging the PR as is cannot be done as it will break every other use where people just want to add additional classes.

@gnat42
Copy link
Contributor Author

gnat42 commented May 12, 2017

I realize this can't be merged as is for many reasons.

Here's the issue. If I specify a form class I have no way to not have the form control class. This may seem insignificant but having to override a form theme to simply remove a class is the wrong solution in my opinion. I can still use the bootstrap theme (css wise not symfony form theme) without having form-control on the input. For example the specific reason I found this issue is because we have a theme built on top of bootstrap. It has a bunch of input sizing classes, they need to be applied without form-control to work. I can't do that without overriding the theme. Unfortunately I think the way the default class is applied for this theme is wrong and changing it would cause a BC but really, creating a form theme is just weird to solve an issue that the form system should just do.

@xabbuh
Copy link
Member

xabbuh commented May 12, 2017

You can still override Bootstrap's CSS for the form-control class in your own CSS files to fit your needs.

@gnat42
Copy link
Contributor Author

gnat42 commented May 12, 2017

yes - I completely and totally understand there are multiples ways around this issue. We've worked around it internally already. You can probably close this - though I still think this should be fixed in a future backwards breaking way/branch. Its nonsense to override it in the other places to fix this.

I filed this bug because when I do this (regardless of the theme involved):

  $builder->add('something',null,['attr'=>['class'=>'some classes']]);

I think it is a bug to get anything but 'some classes' in the output. Its nice to have defaults, but it seems wrong that explicitly setting an option doesn't get exactly that option.

@mvrhov
Copy link

mvrhov commented May 13, 2017

IMHO it's not a bug. The theme is specific to a bootstrap framework and the framework specifies how the theme should look like.

@gnat42
Copy link
Contributor Author

gnat42 commented May 13, 2017

The theme provides the defaults. If you take bootstrap and build a site with it, you are not required to always use class=form-control on your inputs. If you have a situation for whatever reason where you want fine grained control of the class used on the input. It is logical that you would set the class on the symfony form via the attr->class entry. The fact that you cannot remove a class when you explicitly set the class is the issue. The theme has a default yes, but the bug I am reporting is that I cannot remove that without doing crazy amounts of work for overriding themes. If I have a form with 3 inputs and one of them can't have the form-control class applied I have to jump through crazy hoops to override the class I set in the form for that one field. That is a bug IMO.

@xabbuh
Copy link
Member

xabbuh commented May 13, 2017

Well, there are ways to achieve your goal. Either by writing CSS that does not need the class to be absent or by creating a custom theme that extends the built-in one and overrides this widget. Introducing a new config option (which we would need to be backward compatible) IMO is to much work in core to support this edge case.

@ro0NL
Copy link
Contributor

ro0NL commented May 13, 2017

I still think you either opt-in or opt-out from form theming; if you opt-in and use bootstrap as a theme; this means inputs needs a form-control class (from the theme pov).

It's weird only one field should follow a different theme; but in that case there are solutions already (like @xabbuh mentioned).

If overriding becomes a mess we could stil make themes more flexible like mentioned in #22622 (comment)

@gnat42
Copy link
Contributor Author

gnat42 commented May 13, 2017

alrighty

@gnat42 gnat42 closed this May 13, 2017
@mvrhov
Copy link

mvrhov commented May 15, 2017

2nd thing that's documented is that if you need this for one field only you can overwrite it on the spot (same template). or prepare the external template file and include it where needed. IMO you are over-complicating things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants