Hidden input for multiple selects in 1.2-stable #802

Merged
merged 4 commits into from Mar 3, 2012

Projects

None yet

5 participants

@kchien
kchien commented Feb 16, 2012

This incorporates the code from @fbuenemann; I simply wrote a spec to check for the new hidden input.

My 'hidden_input' branch is based off of 1.2-stable and I rebased @fbuenemann's branch on top of mine.

@justinfrench
Owner

Shouldn't the hidden tag come before the select?

if html_options[:multiple]
  create_hidden_field_for_multiple_select(input_name) << select_html
end

?

@felixbuenemann

@justinfrench given that the code worked fine for me in production, I don't think that it matters.

@kchien
kchien commented Feb 17, 2012

I too, thought that the hidden tag should appear before the select, but I just tried it and it worked for me as well.

@justinfrench
Owner

The approach we've taken elsewhere in the code is to put them first, so I'd like to see this switched around and tested pretty please :)

@felixbuenemann

@kchien you can merge/cherry-pick a096d4b from my branch. This puts the input after the label, but before the select.

@kchien
kchien commented Feb 18, 2012

@fbuenemann - Thanks; I cherry-picked that commit , and edited my spec by making the check more specific.
@justinfrench - Please take a look at the modifications and let us know what you think. Thank you!

@justinfrench justinfrench merged commit 5459c19 into justinfrench:1.2-stable Mar 3, 2012
@justinfrench
Owner

merged in, thanks!

@sailor
sailor commented Nov 5, 2012

I can't figure out why do we need hidden fields before each multi-selects? In my application, i systematically get 2 hidden fields before the select, which gives something like that in my controller foo: ["", "", "bar"].

@justinfrench
Owner

@sailor can you please open a new issue with example code to repeat the issue?

@dmitry
Contributor
dmitry commented Nov 11, 2012

I have the same problem as @sailor have. And I also don't understand why do we need hidden fields.

Thank you.

@justinfrench
Owner

@dmitry we need hidden inputs because browsers do not post the attribute if none of the checkboxes or multi-select options were chosen. Mass assignment and ActiveRecord's update_attributes no longer knows if you wanted to unselect all options, or to not update that attribute at all. This is the same problem we have with unchecked checkboxes, and Rails "solved" this with a hidden field. We do the same here. If the choices we've made don't make sense to you, you can quickly change this with a custom input.

The issue of having two hidden fields instead of one is definitely a bug (please, someone, give me a test case!), but I don't want to continually debate the idea that we should have none :)

@dmitry
Contributor
dmitry commented Nov 15, 2012

@justinfrench Thank you for the explanation. I though about that case, actually.

FYI, I just want to describe case, when there are no need for the hidden input. An example is, when using meta_search with scopes. As scopes invoked with the params from the search, and params have empty strings (because of multiselect). To fix that, you should check params for presence in every scope that uses multiselect.

@justinfrench
Owner

@dmitry would love to see a pull request if you've got ideas, thanks!

@dmitry
Contributor
dmitry commented Nov 18, 2012

@justinfrench Investigated that problem a bit.

Hidden input addition was introduced in nashby/rails@9eb6cd6

Then include_hidden was introduced in new rails 4.0: nashby/rails@54a75e1

So hopefully, hidden logic can be removed in the next minor version: 1.3
And after that in new rails it will be possible to use multiple select without hidden field, just including an option: include_hidden: false

What do you think @justinfrench? When the new minor version of formtastic is planned?

Thank you.

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