search

trailblazer / formular

Form builder for Ruby. Fast, Furious, and Framework-Agnostic.
MIT License
81 stars 17 forks source link

Checked will be triggered for values 1, '1', true and 'true' #45

Closed emaglio closed 7 years ago

emaglio commented 7 years ago

As discuss in https://github.com/trailblazer/formular/pull/44 now the checkbox will be checked when the value is 1 or true.

I still don't like the possibility to add checked as option. I mean maybe we should make checked a simple boolean options instead to have checked: 'checked' when the checked options is included. At the moment the checkbox will be checked if I have checked: 'anything' which is confusing from my point of view...

emaglio commented 7 years ago

This old link https://apidock.com/rails/ActionView/Helpers/InstanceTag/to_check_box_tag uses what you have done before and on the recent docs I couldn't find anything specific. It seems that Rails Helper uses the way you have done it but we can be smarter than that maybe. This is what we can do, it is checked if: 1) options[:checked] == "true" or true which has the highest priority or 2) if options[:checked] is not present then it's checked if one of the values 1, '1', true or "true" (third priority) or 3) if options[:checked] is not present, using read_value == options[:checked_value] which will override the point 2 (second priority)

Which means that the default will be the point 2, instead for more specific cases 1 and 3 can get in action.

something like that (super quick, not really tested):

def is_checked?
            return options[:checked] if !options[:checked].nil?
            return reader_value == options[:value] if !options[:checked_value].nil?
            [1, '1', true, "true"].include?(reader_value)
end

does it make sense?

fran-worley commented 7 years ago

Actually, simpleform & rails helpers don't bother with this at all, if not 1 then you have to specify the checked/unchecked value.

https://github.com/plataformatec/simple_form <%= f.input :accepts, as: :boolean, checked_value: true, unchecked_value: false %>

For now, I think we should set checked_value as an optional alias for the value option and leave things at that for now.

See https://github.com/trailblazer/formular/commit/a03f9a3b2cceca95bd78d4348041777a303485e4

fran-worley commented 7 years ago

I'd rather we focussed our time on other aspects like getting the form elements enctype set correctly if the form includes a file field (which will require quite a bit of reworking...)

emaglio commented 7 years ago

Ok, makes sense! :)