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

Problems with .isValid() on required checkboxes #598

Open
krnlde opened this issue Dec 22, 2015 · 13 comments
Open

Problems with .isValid() on required checkboxes #598

krnlde opened this issue Dec 22, 2015 · 13 comments
Labels
Milestone

Comments

@krnlde
Copy link

krnlde commented Dec 22, 2015

Hey guys! I experienced some problems when making checkboxes required (for example terms and conditions).

See here for reference: http://jsfiddle.net/v6d5y3or/

Expected behaviour: Form .isValid() should reflect the checkbox value: checkbox.true = form.true and checkbox.false = form.false

Actual behaviour: Form .isValid() is always true: checkbox.true = form.true and checkbox.false = form.true

I'm not really sure why this happens and one would think that required: true works together with the checked binding.

A temporary solution to me is to implement a custom validator to make it work as expected:

ko.observable(false).extend({
  validation: [{
    validator: (value, params) => Boolean(value)
  }]
})
@krnlde
Copy link
Author

krnlde commented Jan 5, 2016

Can I consider this project dead when, after 2 weeks, nobody answers or even just labels this issue?

@ichepurnoy
Copy link

my best guess is, you have misused ko.validatedObservable.
Namely, you did not extend it. Please see this example.
http://jsfiddle.net/ichepurnoy/evxca44L/
I'm no pro, just trying to figure things out, and it's a pity that the docs not contain many things like this, that are already in the library.
If i'm right, pls , try to use correct syntax for validatedObservable. Maybe get it from library itself.
The project's not dead, and if it even was, we are pretty much stuck with it, as I see no replacement for it currently )

@ichepurnoy
Copy link

I now realised that you have used validatedObservable like in 'Getting Started' .. But check inside the library, maybe docs mislead you on this.
Another thing, check http://jsfiddle.net/ichepurnoy/7brjnewm/, I replaced your input with input type = text.
It works (!), although I also had to change the way the viewmodel is instantiated.
It may be something in knockout itself (not ko.validation plugin), or the way its 'checked' binding works.

@krnlde
Copy link
Author

krnlde commented Jan 6, 2016

I extended the VM correctly in the jsfiddle above. Also I know it works on a text input - but not on a checkbox/radio. That's why I started this issue.

@crissdev
Copy link
Member

crissdev commented Jan 6, 2016

@krnlde The required rule converts the input value to a string and checks its length. For a boolean value the resulting string will always have a length greater than 0, thus making the validation to always pass. (https://github.com/Knockout-Contrib/Knockout-Validation/blob/2.0.3/src/rules.js#L45)
A simple fix would be to check for false to skip the conversion to string (that is a change to https://github.com/Knockout-Contrib/Knockout-Validation/blob/2.0.3/src/rules.js#L27).
Do you want to make a PR to fix this?

@krnlde
Copy link
Author

krnlde commented Jan 6, 2016

But L27 is a shortcut and will return with false if required === true. I'd rather prefer to do another type check as in L32:

if (typeof (val) === 'boolean'`) return val;

What do you think @crissdev

@crissdev
Copy link
Member

crissdev commented Jan 6, 2016

@krnlde Adding false to the existing checks will fail validation if the field is required - which is the desired behaviour. http://jsfiddle.net/v6d5y3or/2/

@krnlde
Copy link
Author

krnlde commented Jan 7, 2016

Alright thanks, I'll do a PR. Should I also build the dist?

krnlde pushed a commit to krnlde/Knockout-Validation that referenced this issue Jan 7, 2016
krnlde pushed a commit to krnlde/Knockout-Validation that referenced this issue Jan 8, 2016
@krnlde
Copy link
Author

krnlde commented Jan 22, 2016

Any news here?

@crissdev
Copy link
Member

Please check your PR. It seems the tests are falling.

krnlde pushed a commit to krnlde/Knockout-Validation that referenced this issue Jan 22, 2016
@krnlde
Copy link
Author

krnlde commented Jan 22, 2016

Not anymore.

@crissdev
Copy link
Member

Looks good. Thanks.

@crissdev crissdev added the bug label Jan 25, 2016
@crissdev crissdev added this to the 2.1.0 milestone Jan 25, 2016
@crushjz
Copy link

crushjz commented Jul 25, 2017

Hello,

I don't think that the value false should be treated as null or undefined .

Example: I have two radio buttons: "Yes" and "No". Selecting yes, the underlying observable is set to true, selecting no, is set to false, and with no selection, the observable is set to null.
The form is valid when the value is either true or false, but invalid when it's null.

With your change the form is invalid even if I have selected No, because the observable is set to false.

Here is the JSFiddle: https://jsfiddle.net/989d6tbb/

Let me know your thoughts.

Thank you

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

No branches or pull requests

4 participants