Closed lcreid closed 2 years ago
My gut feeling on this change, without actually investing the time to technically understand the details, is not that the concept of form_group
has been dropped, but that we just use different, generic class names to accomplish the same thing. So where before we would wrap a form row in <div class="form_group">
, we now wrap the row in <div class="mb-3">
. This could still lead to a code cleanup, but I'd like to take a look at the bootstrap_form
code first.
Please let me know if you think my gut feeling here is off.
See for instance this example from the Bootstrap 5 documentation:
<div class="mb-3">
<label for="exampleFormControlInput1" class="form-label">Email address</label>
<input type="email" class="form-control" id="exampleFormControlInput1" placeholder="name@example.com">
</div>
<div class="mb-3">
<label for="exampleFormControlTextarea1" class="form-label">Example textarea</label>
<textarea class="form-control" id="exampleFormControlTextarea1" rows="3"></textarea>
</div>
It sounds like we have a similar gut feeling.
The way that we handle the form_group
method, along with how we wrap most of the other methods in a form group, is the stuff that I found hardest to understand when I first started working. And it's the part I still find hard to reason about now. So I've always hoped there would be a way to clean it up. Maybe this is the time. On the other hand, if it isn't broken, don't fix it.
If I had the time, I would be investigating where the form_group
method is actually needed, and whether it's needed anymore. I think it was about grouping radio buttons and check boxes, but I haven't had a chance to investigate.
Just remove the method. Why wouldn't you?
At the time I did dive into this issue, but forgot to respond here. The form_group
method is basically at the base of what this gem does. Removing it would remove all ability to render form fields and the additional Bootstrap html around it. We could rename it to, if we follow the Bootstrap class naming, mb_3
, but that doesn’t make sense. We could consider refactoring the code base, but I’m not too sure this would reduce complexity. As it stands the code is a bit hard to understand, but that comes with trying to support all the different types of fields, different layouts and all the goodies that this gem gives you.
So in short, I’d keep it as-is and try to work towards a 5.0 release.
I think this has been resolved. We kept the form_group concept and use the .mb-3 class as default.
Bootstrap 5 drops the concept of
form-group
(https://getbootstrap.com/docs/5.0/migration/#forms). Before we release the final Bootstrap 5 version ofbootstrap_form
, we should at least consider what to do about theform_group
method in the gem.This might be as simple as removing the method. There were some use cases that needed the method in Bootstrap 4 (I think maybe it was for groups of check boxes and/or radio buttons, and maybe others).
Please add your thoughts as comments on this issue. Thanks in advance.