Knockout-Contrib / Knockout-Validation

A validation library for Knockout JS
1.02k stars 379 forks source link

Calling ko.validation.group on object makes all observables validatable #478

Open davhdavh opened 9 years ago

davhdavh commented 9 years ago

Can we please have an option for group that does not make all objects validatable when iterating over the object graph.

The problem is that if you have auto insert span on, then it sometimes breaks the html depending on the span being inserted or not.

crissdev commented 9 years ago

I can't see how a hidden element can break the layout. If you want that span to not be inserted at all then you can use insertMessages option and set it to false.


<!-- Don't insert messages for any of the observables inside this element -->
<div data-bind="validationOptions: {insertMessages: false}">
   <input type="text" data-bind="value: someObservable">
</div>
davhdavh commented 9 years ago

Easy... make a css rule using + On Dec 12, 2014 5:39 AM, "Cristian Trifan" notifications@github.com wrote:

I can't see how a hidden element can break the layout. If you want that span to not be inserted at all then you can use insertMessages option and set it to false.

— Reply to this email directly or view it on GitHub https://github.com/Knockout-Contrib/Knockout-Validation/issues/478#issuecomment-66702478 .

davhdavh commented 9 years ago

Or :first-child, or :last-child, or :nth, or firstoftype, etc. On Dec 12, 2014 6:27 PM, "Dennis Haney" davh@davh.dk wrote:

Easy... make a css rule using + On Dec 12, 2014 5:39 AM, "Cristian Trifan" notifications@github.com wrote:

I can't see how a hidden element can break the layout. If you want that span to not be inserted at all then you can use insertMessages option and set it to false.

— Reply to this email directly or view it on GitHub https://github.com/Knockout-Contrib/Knockout-Validation/issues/478#issuecomment-66702478 .

crissdev commented 9 years ago

While this seems to be easily fixed, we should delay this until grouping and validatedObservable work properly. There are still some bugs related to this and hopefully they will be fixed with the 2.0 release.

FYI, I ran all the tests with the following change and they passed.

if (obj.isValid) {
  // Don't extend the observable with {validatable: true} 
  // and skip adding it to validatables
  context.validatables.push(obj);
}

I will look more into this and see what options we have and how can it better be addressed. The progress of 2.0 release is available here.

I will update this issue once I find a way to proper way to tackle this. Other opinions are welcome.

davhdavh commented 9 years ago

I would be ok with the default being the current way of working. But I would like a very simple way to insert the html element somewhere else.

crissdev commented 9 years ago

I would be ok with the default being the current way of working. But I would like a very simple way to insert the html element somewhere else.

Are you after something similar to #456?

davhdavh commented 9 years ago

yes

On Sat, Dec 27, 2014 at 8:25 PM, Cristian Trifan notifications@github.com wrote:

I would be ok with the default being the current way of working. But I would like a very simple way to insert the html element somewhere else.

Are you after something similar to #456 https://github.com/Knockout-Contrib/Knockout-Validation/issues/456?

— Reply to this email directly or view it on GitHub https://github.com/Knockout-Contrib/Knockout-Validation/issues/478#issuecomment-68178478 .

Dennis

kidkuro commented 9 years ago

We have been using Knockout-Validation with great results on a project that needs to be accessible (WCAG 2.0 AA compliant). However, this behaviour is causing a major issue for us.

We associate validatable inputs with their error messages by adding an aria-describedby attribute to the input, with the value being the id of the error message. The error message id is derived from the observable.

We use a group to toggle whether an alert is included to indicate to the user (potentially using a screen reader) that there are errors that need to be fixed.

When we have multiple radio buttons bound to the same observable, we don't want to validate the observable (i.e. each individual radio button), however we end up with multiple error messages inserted in the DOM - all with the same id. This is invalid HTML, and hence fails WCAG 2.0.

We could try to hack around this, but since non-validatable observables have no validators, then making them validatable when creating a group doesn't seem to have any positive effect. If an observable has no validation, shouldn't it simply not be added to the validatables array?

Even if there are use cases where this is helpful, for projects (such as ours) where by default this is not desired, having to fill the view with insertMessages: false around every non-validatable observable binding, or even having to apply the proposed extender to every non-validatable observable, adds a lot of unnecessary clutter. One of the great achievements of Knockout-Validation is it's terse but powerful API.

Could I propose simply adding a configuration option that toggles this behaviour? Then it can be set once, and then all the unnecessary (and potentially invalid) HTML simply won't get generated.

crissdev commented 9 years ago

@kidkuro Thanks for such a thorough explanation.

Apart from insertMessages: false you could pass an object or array to ko.validation.group to specify only the observables you are interested. Of course, this may not be as simple as ko.validation.group(this, {/*options*/}) but it will work.

If you need this feature asap and don't want to wait until this issue is fixed you might want to take a look at #224.

kidkuro commented 9 years ago

@crissdev Thanks for your response and your suggestions.

Out of interest, why does the group turn observables without any attached validators into validatable observables? Is there something important I am missing here? To me it seems like that won't do anything useful.

crissdev commented 9 years ago

Making observable validatable will ensure consistency when rules will be applied. Of course, the extender is not so useful if the grouping option observable is false - for this case it make sense not to extend the observable with validatable.

I've created http://jsfiddle.net/qr0o3tt1/ just to demonstrate what happens if the observable is not extended with validatable.

This issue might get in 2.1.0 milestone but it's too early to be sure of it. I'll post updates here once I go through the list of issues again.