Inline errors for toggles

[gkop]

As far as I can tell, master is missing inline errors for toggles.

I wrote a quick fix so that I can use this library on a project, but it's not pull-worthy: https://github.com/coshx/twitter_bootstrap_form_for/commit/36ca5dbae52da7a9df59f364092f05dc5441c0ee

If I understood the reason for the toggle abstraction, I'd try to come up with a real fix.

[stouset]

I'll commit a fix tomorrow morning. Thanks for the report and code!

[stouset]

"Tomorrow morning" was perhaps a bit ambitious. I hope you'll accept "Today Evening".

[stouset]

Hm. This actually is somewhat of an issue. Twitter Bootstrap only styles errors with a red highlight if wrapped with a div having a class "clearfix". But the clearfix divs also create a bunch of extra margins.

Another issue is the radio buttons — they differ from checkboxes, in that the whole group should be highlighted as invalid. With checkboxes, only one ought to be. Right now I'm handling them in a toggles block, but they would have to be separated out into two separate components.

I'm not sure what to do about this.

[stouset]

The reason for the TOGGLES abstraction is because the code to generate a list of checkboxes and radio buttons has been identical... so far, at least. With radio buttons, it makes sense to wrap the <ul> with an error div, but not so much with checkboxes. I could separate them out and have separate form.checkboxes and form.radio_buttons blocks to handle the special casing between the two, but I'm honestly not sure how to handle errored checkboxes. Twitter Bootstrap has no examples of that, and I suspect the issue simply hasn't come up for them.

[stouset]

Any further ideas on this issue? I'm in a holding pattern until I have a better idea of how to solve it.

[gkop]

Thanks for looking into this! Checkboxes are important also. When I figure out a longer term solution for my project, I'll share it, but it might be a couple weeks.

[stouset]

I'll hold off on anything for the time being.

[derekprior]

Meh, I'd suggest doing something simple that will work. Adding the error class to the ul for checkboxes would cause the whole thing to be highlighted. I think that's better than nothing. It's not a perfect solution, but it's forward movement, I think.

[stouset]

I'm gravitating towards not supporting these. My suspicion is that having errors on checkboxes or radio buttons is probably just bad UI. In the classic example of "You must agree to the terms and conditions...", the appropriate way to handle it is how Github does: "By clicking on 'Create an account' below, you are agreeing...".

Is this a showstopper for anyone?

I'm open to counter arguments (hence leaving this bug open for comment), but given Twitter Bootstrap's lack of support or guidance for this, plus my own predilections, I'm leaning towards not supporting this behavior.

[derekprior]

Seems reasonable to me.

[gkop]

I object on the basis of consistency :)

Seriously, if we can't get inline errors for all the standard inputs, I'd rather none at all. Let's not deceive people migrating from eg. formtastic by claiming to support inline errors.

I agree the terms and conditions checkbox case is marginal, but what about these, not uncommon, cases?

• you have n checkboxes, at least x of which, but not more than y, must be checked for the form to validate.
• you have a yes and a no radio button, the form is valid if either yes or no is selected, but invalid if neither is selected.

[stouset]

I just played around with errors for checkboxes and radio buttons on the latest Bootstrap examples available. It looks like the only styling that works is to add an "error" class to the <div class="clearfix"> that surrounds the entire list of checkboxes and radio buttons. I wasn't able to get appropriate error styling of any kind from any other combination of classes on various elements.

This now causes a problem. The toggles method is responsible for emitting this div, but to give it a class="error", it would need to know the attributes on the object that are exposed through the inputs inside of it.

I could conceivably split out toggles into two methods: check_boxes, and radio_buttons. Converting my example from the README:

/ group of radio buttons
= user.radio_buttons :email, 'Email Preferences' do
  = user.radio_button :email, 'HTML Email', :html, :checked => true
  = user.radio_button :email, 'Plain Text', :plain

This would work fine. The radio_buttons method could check for the presence of an error on the email attribute, and do the right thing. But checkboxes...

/ group of checkboxes
= user.check_boxes $WHAT_GOES_HERE, 'Agreements' do
  = user.check_box :agree,   'I agree to the abusive Terms and Conditions'
  = user.check_box :spam,    'I agree to receive all sorts of spam'
  = user.check_box :spammer, 'I agree to let the site spam others through my Twitter account'

The checkbox group encompasses multiple attributes. I could specify all the attributes as an array passed to the check_boxes method, but this is non-DRY and will surely lead to bugs where some invalid checkbox attribute isn't included in the list.

I could conceivably support the radio_buttons approach now, and punt on checkbox errors until the answer becomes clearer (or Twitter Bootstrap explicitly supports such a use). I suspect the "you must choose one of these radio buttons, but neither should be selected by default" is much more prevalent than "you have #{n} checkboxes, of which some number and/or combination must be checked".

[gkop]

I like that idea a lot, very pragmatic.

[derekprior]

I would argue that those are really three separate checkboxes with independent errors. I'd probably render them as such by using 3 user.toggles (or user.check_boxes under this proposal) without the group having a label.

In my experience, at least, the much more likely use case for multiple checkboxes is when they are mapped to a single field.

/ group of checkboxes
= user.check_boxes :roles 'Roles' do
  = user.check_box :roles,   'Administrator'
  = user.check_box :roles,   'Moderator'
  = user.check_box :roles,   'User'

In this case, it'd work just like the radio button example, right? FWIW, I couldn't get this arrangement to be marked up correctly with current version last night, so I had to work around it. Clicking the labels for the independent checkboxes always selected the first checkbox because the checkboxes all have the same name. Checkboxes should probably be wrapped by the label tag instead. I ended up doing:

= f.toggles 'Roles' do
  %label
     %li= check_box_tag "user[roles][]", 'Administrator'
   %label
      %li= check_box_tag "user[roles][]", 'Moderator'
   %label
      %li= check_box_tag "user[roles][]", 'Moderator'
= f.hidden_field_tag 'user[roles][]'

I considered opening it as a separate issue, but if you're consiering overhauling the checkbox code here anyway...

[stouset]

From the Rails documentation, you probably need to be using check_box_tag for array-type checkboxes anyway.

[stouset]

Ok. I plan on doing as I said above: support this for radio buttons, but not currently for checkboxes. Will try to get a patch out this weekend.

[lukesaunders]

This still seems to be open and is causing me problems. I made a branch and fixed it for check_boxes only like so:

https://github.com/lukesaunders/twitter_bootstrap_form_for/commit/61230f9f5a875a1e0ad423238d90647096000926

The clearfix does cause some margin but I think it looks fine. I have a screenshot here:

Is this okay? If so maybe we could do the split of radios and checkboxes?

[stouset]

Fair enough. Issue a pull request and I'll merge it in.

[lukesaunders]

Done. Happy to help with something less hacky if you prefer.

[stouset]

2.0 is going to be released soon. Kind of in a holding pattern until it comes out.

[lukesaunders]

Makes sense, thanks.