Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline errors for toggles #9

Open
gkop opened this issue Oct 18, 2011 · 21 comments
Open

Inline errors for toggles #9

gkop opened this issue Oct 18, 2011 · 21 comments
Assignees
Milestone

Comments

@gkop
Copy link

gkop commented Oct 18, 2011

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: coshx@36ca5db

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

@ghost ghost assigned stouset Oct 18, 2011
@stouset
Copy link
Owner

stouset commented Oct 18, 2011

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

@stouset
Copy link
Owner

stouset commented Oct 18, 2011

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

@stouset
Copy link
Owner

stouset commented Oct 19, 2011

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
Copy link
Owner

stouset commented Oct 19, 2011

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
Copy link
Owner

stouset commented Nov 6, 2011

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

@gkop
Copy link
Author

gkop commented Nov 6, 2011

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
Copy link
Owner

stouset commented Nov 6, 2011

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

@derekprior
Copy link
Contributor

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
Copy link
Owner

stouset commented Nov 15, 2011

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
Copy link
Contributor

Seems reasonable to me.

@gkop
Copy link
Author

gkop commented Nov 15, 2011

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
Copy link
Owner

stouset commented Nov 15, 2011

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
Copy link
Author

gkop commented Nov 15, 2011

I like that idea a lot, very pragmatic.

@derekprior
Copy link
Contributor

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
Copy link
Owner

stouset commented Nov 15, 2011

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

@stouset
Copy link
Owner

stouset commented Nov 17, 2011

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.

@ghost ghost assigned stouset Nov 17, 2011
@lukesaunders
Copy link
Contributor

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

lukesaunders@61230f9

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

Error

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

@stouset
Copy link
Owner

stouset commented Jan 31, 2012

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

@lukesaunders
Copy link
Contributor

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

@stouset
Copy link
Owner

stouset commented Jan 31, 2012

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

@lukesaunders
Copy link
Contributor

Makes sense, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants