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

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

Open
davhdavh opened this issue Dec 7, 2014 · 11 comments
Open

Comments

@davhdavh
Copy link
Contributor

davhdavh commented Dec 7, 2014

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
Copy link
Member

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
Copy link
Contributor Author

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
#478 (comment)
.

@davhdavh
Copy link
Contributor Author

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
#478 (comment)
.

@crissdev
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

davhdavh commented Jan 6, 2015

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
#456?


Reply to this email directly or view it on GitHub
#478 (comment)
.

Dennis

@kidkuro
Copy link

kidkuro commented Feb 10, 2015

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
Copy link
Member

@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
Copy link

kidkuro commented Feb 10, 2015

@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
Copy link
Member

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.

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

No branches or pull requests

3 participants