Add support for inline inputs

[stouset]

The example page for Twitter Bootstrap includes examples for multiple input fields inlined. We should explicitly support this.

[theunraveler]

I would suggest something like f.inputs :inline => true.

[stouset]

Possibly. All of the form helpers inside that block need to change how they output, which makes this a PITA to deal with. Then I have to figure out how best to display errors for multiple inputs in that one area...

[derekprior]

how about:

f.inline_fields 'my label' do |inline|
  inline.text_field :start_time
   -
  inline.text_field :end_time
  link_to "Timezones", "#"
end

For error handling, I would suggest using the help-block style where the errors for each field would print below the inputs in the block. If possible, highlight only the field with the error, otherwise highlight the entire block. After all, the reason for displaying the elements inline would be that they are considered a single element.

This is probably my one hangup about moving to this gem from simple_form (which is far from perfect with bootstrap). I have several forms where what can be though of very easily as a single logical element has to be presented in multiple inputs but doesn't deserve multiple labels/lines.

[stouset]

I like the approach. Unfortunately, it's a non-trivial amount of work (basically, a new FormBuilder subclass) for something I don't currently use. So, you have two options: 1) wait for me to have time to do it, or 2) submit a patch. The latter would be very much welcomed. :) If you don't have time, I'll get to it but it may take me a bit.

[theunraveler]

Why does this need a new FormBuilder? It seems like you could just do:

f.inline_fields 'label' do
  f.text_field :start_time
  ...
end

All we're talking about is stylistic grouping, not new functionality.

[stouset]

The existing text_field method wraps everything in a <div class="clearfix">, creates a <label>, another <div> to go around the field, and finally the <input> itself.

The wrapped text_field method (and other input methods) would only emit the <input> tag itself. In order to accomplish this, the inline method would have to output the wrapper divs (<div class="clearfix">, <div class="input">, and <div class="inline-inputs">, all nested) and yield a FormBuilder for the pared-down input methods.

Actually, thinking on it, it could just yield the normal FormBuilder, since the inline-input methods don't need to do any extra magic.

[derekprior]

That's what I was thinking. I may have some time tomorrow night to try a patch.

[stouset]

Sorry to punt on so much today. I've been swamped for the last two weeks, and haven't had the spare cycles to do much development on the side. Anything that's not a bug has to wait. :(

[derekprior]

Ok, I'm trying to implement this, but not having much luck. Here's what I've got:

#
  # Creates bootstrap wrapping before yielding a plain old rails builder
  # to the supplied block.
  #
  def inline_inputs(label = nil)
    template.content_tag(:div, :class => 'clearfix') do
      template.concat template.content_tag(:label, label)
      template.concat template.content_tag(:div, :class => "input") {
        yield ActionView::Helpers::FormBuilder.new @object_name, @object, @template, @options, @proc if block_given?
      }
    end
  end

But the yielded object still seems to output the bootstrap divs, labels, etc. I'm pretty new to this. I'm going to keep trying, but if you see something I'm doing obviously wrong, let me know. The FormBuilder initialization must be wrong.

[stouset]

That looks reasonable to me. I'll have more time to look after work tonight.

[derekprior]

Making a little progress. It actually works if I use the yielded object to render a text field directly. But if I use it like so:

= inline_inputs do |inline|
  = inline.fields_for :associated do |assoc|
    = assoc.text_field :name

then assoc is a TwitterBootstrapFormFor::FormBuilder. I thought it would be a regular FormBuilder as a result of spawning it from inline.fields_for... hmm...

[derekprior]

Okay, looks like I just had to override the :builder option in the hash passed to the initializer. Pull Request #25 submitted.

[stouset]

Pull request merged. Release will go out in a day or two when I have time to do some thorough testing.

[derekprior]

Cool - about that... Do you have a testing framework preference? I was going to add some specs (rspec), but didn't want to be presumptuous.

[stouset]

I was intending to use MiniTest. It's built into Ruby 1.9, is fast, and has an RSpec-like DSL syntax.

[stouset]

I've been tweaking this pull request, and came across an unfortunate limitation. If there are errors on inline fields, they won't show up (because we're using the default FormBuilder).

We might need to actually make a second FormBuilder later that doesn't emit the div wrappers, but handles the error messages.

[stouset]

I've released this in 1.0.4. I did, however, change the name of the method to inline rather than inline_inputs.

[derekprior]

Hmmm - good point about the limitation. I haven't tested that part - does it mean that the user is on their own for coming up with a way to add the styling in a manner consistent with the rest of the bootstrap fields?

I take it that the full error messages display on the form would also be wrong. That's a bummer.

[stouset]

Yes. Those fields are rendered exactly as they would be outside of the t_b_f_f gem.

[derekprior]

The change you made to the implementation causes problems when spawning child form builders inside the block passed. In my case, I do this when rendering fields for an object that the form accepts_nested_attributes_for My implementation didn't exhibit the same issues. I made a test repository to showcase this issue.

As you can see here, I make a call to fields_for inside the inline block. When those fields are rendered the field name is set improperly. The nesting is one level too deep. In my sample app, the comment text field is rendered with the name post[post][comments_attributes][0][text] which has an exta [post] in it. This means that data is silently discarded by the controller.

This doesn't happen with my older implementation that simply yielded a form builder back to the caller.

[derekprior]

In fact, the reproduction of this issue can be simplified even further, eliminating the nested fields. The field names generated for ANY fields rendered with the inline builder are wrong. Check out my exmaple. Those fields are rendered with the names post[post][title] and post[post][body] when they should be post[title] and post[body] respectively.

[stouset]

Crap.

I chose that approach because it seemed less brittle than creating the builder myself. Will push a fix ASAP.

[theunraveler]

Yep, I'm going to be that guy.

[stouset]

https://github.com/stouset/twitter_bootstrap_form_for/issues/8 :(

[theunraveler]

(I was just kidding. I have definitely been guilty of the same.)

[stouset]

Hoping I get some time over the Thanksgiving break... been crazy busy, unfortunately.

[stouset]

dc6ac1fe97960499ea2c6a6d276ce756e97c2b95

This should fix the issue. I'll push a new release once I get confirmation from you guys. :)

[derekprior]

Tested it with my test repo and verified this fixes the issue.

[stouset]

Released.

[theunraveler]

This doesn't work if ActionView::Base.default_form_builder is TwitterBootstrapFormFor::FormBuilder. The new form builder is another instance of t_b_f_f. I think that line 79 should be

self.options.merge(:builder => ActionView::Helpers::FormBuilder),

while line 76 remains the same.

[stouset]

Is anyone setting that? I hadn't considered that anyone ever would. :)

[theunraveler]

Definitely. That way I don't have to set it on every form_for if I want all of my forms to use Bootstrap.

[stouset]

You can just use twitter_bootstrap_form_for, you know. Although it's kind of flattering you've made this the default?

Point taken. I figured it was more correct to use whatever someone had configured as the default, but I can't actually think of a case in which it wouldn't be better to use the base FormBuilder.

[stouset]

8a767955d64317ea795c5da68478d666c62ab67a