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

form_group, rows, and columns #449

Closed
lcreid opened this issue Mar 8, 2018 · 5 comments
Closed

form_group, rows, and columns #449

lcreid opened this issue Mar 8, 2018 · 5 comments
Assignees
Milestone

Comments

@lcreid
Copy link
Contributor

lcreid commented Mar 8, 2018

With Bootstrap 3, .form-group contained the styles of a .row via a mixin (see the "Horizontal forms overhauled" bullet at https://getbootstrap.com/docs/4.0/migration/#forms-1). Bootstrap 4 does not mix in .row to .form-group. Also, in Bootstrap 4 there are two row classes: .row, and .form-row which has smaller gutters.

Even with Bootstrap 3, there was issue #89 (and may have been others) that was raised about the challenge of making a horizontal form. Since .row was baked in to form-group, it was difficult to fix.

I'd like to suggest that for version 4.0, the form_group helper should not put any additional styles, i.e. .row on a div.form-group, unless the layout is :horizontal, either at the form_group level or the bootstrap_form_for level.

If the layout is :horizontal, form_group should:

  • add nothing to the classes, if the user specified either row or form-row in the wrapper_class: or wrapper: { class: } options
  • add form-row (not row) to the class, if the user did not specify either row or form-row in the wrapper_class: or wrapper: { class: } options

The reason for the above is because .col has to be an immediate child of .row. Since we generate a div.form-group for almost all inputs, we have to put the row class(es) on the div.form-group.

I think it would be worthwhile to highlight this change in the UPGRADE-4.0 document, as I suspect that people have tried various tricks to get horizontal forms to lay out nicely.

It's worth noting that https://github.com/twbs/bootstrap/blob/fb60a4a9867eb1fc18cf310de2bbb8f21e262e6b/scss/_forms.scss#L172-L175 says .form-group is for vertical, AKA, default layout forms, and that horizontal forms should use the predefined grid classes. That might open the door to another option: Not generate the div.form-group at all if layout: :horizontal and leave it completely up to the user. I haven't thought this option through as much as the above, but I think it's worth thinking about.

@lcreid lcreid added this to the 4.0.0 milestone Mar 8, 2018
@mattbrictson
Copy link
Contributor

I think this is worth doing for 4.0.0. I agree with this proposal:

the form_group helper should not put any additional styles, i.e. .row on a div.form-group, unless the layout is :horizontal, either at the form_group level or the bootstrap_form_for level.

In the case of :horizontal, I think we should add row, not form-row. The latter seems more appropriate to complex custom layouts. In the standard 2-column horizontal form layout, I think we want the normal gutter width.

Otherwise I am completely in agreement with this proposal.

@lcreid lcreid self-assigned this Apr 15, 2018
@lcreid
Copy link
Contributor Author

lcreid commented Apr 15, 2018

It appears the code already does most of what @mattbrictson agreed to. The only part missing is to allow the user to choose form-row if they want.

@abrambailey
Copy link

abrambailey commented Apr 24, 2019

The conversation above was slightly confusing to me, but here's the look I was going for:

image

And here's how I accomplished it:

<div class='form-row'>
  <%= f.text_field :first_name, wrapper_class: 'col-md-6' %>
  <%= f.text_field :last_name, wrapper_class: 'col-md-6' %>
</div>

Please let me know if I have done this correctly, thanks!

@lcreid
Copy link
Contributor Author

lcreid commented Apr 26, 2019

That's exactly the way you should do it. It was the intent of the final decision on this issue. Had we carried the Bootstrap 3 behaviour into the Bootstrap 4 version of bootstrap_form, your layout might not have even been possible. Sorry the above conversation was a little convoluted.

@abrambailey
Copy link

Thanks for all you do on this awesome gem @lcreid !

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

No branches or pull requests

3 participants