Using :label => false results in error #34

Closed
cpytel opened this Issue Aug 11, 2009 · 17 comments

Comments

Projects
None yet
4 participants

cpytel commented Aug 11, 2009

Attempting to use :label => false results in the following error:

You have a nil object when you didn't expect it!
You might have expected an instance of Array.
The error occurred while evaluating nil.+

Contributor

grimen commented Aug 11, 2009

Could you please show the stack trace? I haven't got this error...

cpytel commented Aug 11, 2009

Yes, I was just going to look at this, get the stack trace and also see if I could write a failing spec for it as well. I'll comment with more soon.

cpytel commented Aug 11, 2009

Here is the backtrace:

/Users/cpytel/projects/hoptoad/vendor/gems/justinfrench-formtastic-0.2.0/lib/formtastic.rb:463:in `input_simple'
/Users/cpytel/projects/hoptoad/vendor/gems/justinfrench-formtastic-0.2.0/lib/formtastic.rb:894:in `inline_input_for'
/Users/cpytel/projects/hoptoad/vendor/gems/justinfrench-formtastic-0.2.0/lib/formtastic.rb:99:in `send'
/Users/cpytel/projects/hoptoad/vendor/gems/justinfrench-formtastic-0.2.0/lib/formtastic.rb:99:in `input'
/Users/cpytel/projects/hoptoad/vendor/gems/justinfrench-formtastic-0.2.0/lib/formtastic.rb:98:in `map'
/Users/cpytel/projects/hoptoad/vendor/gems/justinfrench-formtastic-0.2.0/lib/formtastic.rb:98:in `input'

cpytel commented Aug 11, 2009

just upgraded to 0.2.1 and the same problem occurred, same line number.

Contributor

josevalim commented Aug 12, 2009

Cpytel, your fix is good. Beware that there are other inputs where the same error can happen.

Owner

justinfrench commented Aug 12, 2009

Awesome!

I wasn't sure that we supported :label => false, so I went looking for it in the documentation and specs, and I can't find any reference either. So this is really a feature request, not a bug/fix IMHO.

I have two concerns with this feature addition:

  1. The supplied CSS file expects the labels and legends to always be there for positioning.

  2. It's "correct" and "accessible" for an input to have a corresponding label tag.

Alternatively, if you don't want the label, you can choose to hide the label with a CSS declaration like "display:none;" (or any other CSS hiding technique) , which addresses both issues:

  1. if you're hiding the label with CSS, it means you're either writing your own CSS, or you're actively choosing to add to the default CSS, which means you can address any display issues that come up

  2. the label is still there in the markup for non-visual user agents (or even for Cucumber specs, which use labels to identify inputs, I think?)

For now, I'm tagging this with 1.x and leaving it open, happy to continue the discussion here or in the Google group!

cpytel commented Aug 12, 2009

Sorry justin, but I think you're mistaken about it being a feature request. The functionality is specified in the specs, line 327 (of master, not my fork), here: http://github.com/justinfrench/formtastic/blob/1e150df66c8e89cddbd31feeedca9f37827be140/spec/formtastic_spec.rb#L327

I discovered it in the specs, tried to use it, and it didn't work.

We've had two instances in our apps where we've needed to use :label => false, both have been where we had another label on the form for the input element because the label markup needed to be customized, or in a different place than could be accomplished through css alone. In that case, we don't want to hide the original label with CSS because then there would be two.

cpytel commented Aug 12, 2009

jose, good to know. Looks like you wrote the initial functionality and specs for :label => false - do you think that the label method should be changed to return an empty string instead of nil when false is given so that it doesn't cause further issues in other places?

Owner

justinfrench commented Aug 12, 2009

Guess I need to look at the CSS for that case (and the docs!)

Contributor

josevalim commented Aug 12, 2009

@justin, sorry mate, my bad here, I will put more attention into the docs. I've used this option with success, so I don't think we need some CSS. :)

@cpytel, yeah, we can do this way since this option is supposed to be handled by formtastic only internally.

Owner

justinfrench commented Aug 12, 2009

no probs

cpytel commented Aug 12, 2009

ok, I'm modified my fork to return a blank string instead of nil and all specs still pass (including the extra one I added).

I went to go modify the docs, and actually, it looks like it was already documented on the label method:

     # * :label - An alternative form to give the label content. Whenever label
    #            is false, a blank string is returned.

Looks like we just unwittingly made it match the docs :) Don't know if we want to document this behavior in other places as well.

cpytel commented Aug 13, 2009

Do you want me to send a pull request, make a patch, or will you just pull from my fork?

Contributor

josevalim commented Aug 13, 2009

I've tried to pull, I couldn't. Usually once per week I run through formtastic issues and fix a couple of them. Next time I do it, I will fix it and ask Justin to release a new gem.

Contributor

josevalim commented Aug 25, 2009

Fixed.

This issue was closed.

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