Add support for inline inputs #2

Closed
stouset opened this Issue Oct 1, 2011 · 34 comments

Comments

Projects
None yet
3 participants
Owner

stouset commented Oct 1, 2011

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

stouset was assigned Oct 1, 2011

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

Owner

stouset commented Oct 31, 2011

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...

Contributor

derekprior commented Nov 14, 2011

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.

Owner

stouset commented Nov 15, 2011

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.

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.

Owner

stouset commented Nov 15, 2011

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.

Contributor

derekprior commented Nov 15, 2011

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

Owner

stouset commented Nov 15, 2011

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. :(

Contributor

derekprior commented Nov 15, 2011

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.

Owner

stouset commented Nov 15, 2011

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

Contributor

derekprior commented Nov 15, 2011

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...

Contributor

derekprior commented Nov 15, 2011

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

Owner

stouset commented Nov 15, 2011

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

stouset closed this Nov 15, 2011

Contributor

derekprior commented Nov 15, 2011

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.

Owner

stouset commented Nov 15, 2011

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

Owner

stouset commented Nov 20, 2011

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.

Owner

stouset commented Nov 20, 2011

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

Contributor

derekprior commented Nov 21, 2011

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.

Owner

stouset commented Nov 21, 2011

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

Contributor

derekprior commented Nov 22, 2011

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.

Contributor

derekprior commented Nov 22, 2011

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.

Owner

stouset commented Nov 22, 2011

Crap.

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

Yep, I'm going to be that guy.

Tests

Owner

stouset commented Nov 22, 2011

#8 :(

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

Owner

stouset commented Nov 23, 2011

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

Owner

stouset commented Nov 25, 2011

dc6ac1f

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

Contributor

derekprior commented Nov 28, 2011

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

Owner

stouset commented Nov 28, 2011

Released.

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.

Owner

stouset commented Dec 1, 2011

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

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

Owner

stouset commented Dec 2, 2011

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.

Owner

stouset commented Dec 2, 2011

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