Skip to content

never render fantom hidden input (should close #697) #702

Closed
wants to merge 2 commits into from

3 participants

@dnagir
dnagir commented Oct 1, 2011

See #697.

I did not use hidden_fields? as I it doesn't work exactly as I expect.
First of all, "?" method should return boolean, but it just returns the value from options which often is nil.

Converting it to !!options[:hidden_fields] will do the job but it won't be correct as in this case:

  • :hidden_fields => false will return false
  • :hidden_fields => true will return true
  • :hidden_fields => nil will return false (when it is nil it should probably be true - render hidden by default).
@yabawock
Collaborator
yabawock commented Oct 1, 2011

See my comment in #697 - I'm against this patch in it's current form. It changes default behaviour and breaks clearing "simple" has_many associations

If there's the need for a third state of the hidden_fields option we should try to keep it backwards compatible and add some more intelligence to parsing the hidden_fields option value

So we could do something along the lines of:

:hidden_fields => true will render collection and per checkbox hidden inputs
:hidden_fields => :enabled will render collection and per checkbox hidden inputs

:hidden_fields => nil will render collection hidden input but not the per checkbox hidden inputs
:hidden_fields => false will render collection hidden input but not the per checkbox hidden inputs
:hidden_fields => :disabled will render collection hidden input but not the per checkbox hidden inputs

:hidden_fields => :never will not render either collection or checkbox hidden inputs

@dnagir
dnagir commented Oct 1, 2011

I don't see it breaking anything. All the checkbox_input specs are still passing.
But using a symbol for hidden_fields would be a good idea as we could precisely define what we want to render.

@dnagir
dnagir commented Oct 2, 2011

Does this make more sense? The only change is the new option :never. It should never render any hidden inputs.
Otherwise it should work as previously with false, true and nil.

@justinfrench
Owner

Closing this since #705 includes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.