Skip to content
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

Allow overriding of default configured form_class #1109

Merged

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Jul 23, 2014

This is especially useful with bootstrap forms where you might want to switch to a form-inline layout even though the default is a form-horizontal.

e.g.

config.form_class = "form-horizontal" # configured default
simple_form_for @user, form_class: "form-inline"

I recognize that this was (somewhat) already proposed and closed in #657 and #639, but I feel like this new implementation is more straight forward, cleaner, and more discoverable without passing around non-standard html attributes. It's merely a config override.

Just throwing this out there.

@sman591
Copy link

sman591 commented Jul 23, 2014

👍 This is a simple solution that does exactly what Boostrap needs.

Example: I have an "inline" login form in my nav bar, however every other form on my application is "horizontal". Before this, an entire new helper would be needed just to change the form class. Now, all that would need to change is one variable - simple and prepared for future simple_form versions.

@rafaelfranca
Copy link
Collaborator

I'm reluctant to add a new option to set the form class. This will cause confusion with the html: { :class } option of simple_form_for. I don't see why use both in the same form, that said I prefer to keep using the same html: { :class } option of form_for and do not create a new option.

What we could improve is, if html: { :class } is not provided we use SimpleForm.form_class, otherwise we use only html: { :class }.

@rmm5t
Copy link
Contributor Author

rmm5t commented Jul 23, 2014

What we could improve is, if html: { :class } is not provided we use SimpleForm.form_class, otherwise we use only html: { :class }.

I'd be all for that. Plus, it would also better mimic the behavior of the html: { :novalidate } override.

I'd be happy to turn this pull-request into that implementation if you're open to it.

@rafaelfranca
Copy link
Collaborator

Please go ahead. It just can't happen in a 3.x version since it may break other people application. Maybe if we want to release in the next 3.x version we should add an option to enable and disable this new behaviour. I don't like the idea of adding a new global option but I don't have a better idea.

@rmm5t
Copy link
Contributor Author

rmm5t commented Jul 23, 2014

@rafaelfranca One more thing. If we make this change, I think SimpleForm.form_class should be deprecated in favor of SimpleForm.default_form_class.

Perhaps in the interim for 3.x, why don't we add default_form_class and that is the only thing overridden. Later, we can just remove support for form_class.

In other words, here's how I would configure my application for now in 3.x:

# config/initializers/simple_form.rb
config.default_form_class = "form-horizontal"
config.form_class = :simple_form # possibility for a deprecation warning now

Result:

# defaults
simple_form_for @thing
# <form class="simple_form form-horizontal" ... >

# inline override
simple_form_for @thing, html: { class: "form-inline" } 
# <form class="simple_form form-inline" ... >

@rafaelfranca
Copy link
Collaborator

Excellent idea 👍

rmm5t added 2 commits July 23, 2014 18:04
This is especially useful with bootstrap forms where you might want to
switch to a `form-inline` layout even though the default is a
`form-horizontal`.

This deprecates SimpleForm.form_class in favor of
SimpleForm.default_form_class.

e.g.
config.default_form_class = "form-horizontal"
simple_form_for @user, html: { class: "form-inline" }
@rmm5t
Copy link
Contributor Author

rmm5t commented Jul 23, 2014

@rafaelfranca Pull-request force-pushed to cover the new default_form_class option that we discussed. Code review and pull very much appreciated.

I made an update to the changelog too, but as a separate commit in case you didn't want me to make that update.

@rmm5t
Copy link
Contributor Author

rmm5t commented Aug 1, 2014

@rafaelfranca Any word on whether you think this pull-request can make it in before an official 3.1.0 release?

@rafaelfranca
Copy link
Collaborator

For me it is good. I remember @carlosantoniodasilva has some concerns but I don't remember what. I'll merge as soon I have time to work on Simple Form.

@futhr
Copy link

futhr commented Sep 18, 2014

Loved to see this merged 👍

@rafaelfranca rafaelfranca merged commit 2e61a54 into heartcombo:master Nov 19, 2014
rafaelfranca pushed a commit that referenced this pull request Nov 19, 2014
Allow overriding of default configured form_class
@rafaelfranca
Copy link
Collaborator

Thanks ❤️ 💚 💙 💛 💜

@rmm5t rmm5t deleted the simple_form_for-config-overrides branch November 19, 2014 17:02
amatriain added a commit to amatriain/feedbunch that referenced this pull request Dec 1, 2014
…e new config option "default_form_class".

For more about the rationale behind the change, see:

heartcombo/simple_form#1109
@bquorning
Copy link

I was surprised to see that there’s no link between this PR and the issue #1217. Well, now there is :-)

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

Successfully merging this pull request may close these issues.

5 participants