Run observable validations when observable change. #58

Merged
merged 3 commits into from Mar 12, 2014

Projects

None yet

6 participants

@luizfar
  • Also adds a configuration to allow the validation to run only on input changes, if desired.
@jpbochi

Will we have to set validatesOn: 'inputChange' everywhere in reach? I'm confused.

@bradgignac
racker member

Can we just subscribe to observable changes and perform validation then? It seems like this would work in both circumstances.

@luizfar

@jpbochi, the idea would be to use validatesOn: 'change' (which is the default) everywhere except for places where the observable is bound to an input with data-bind="valueUpdate: 'afterkeydown'" (group name on the AutoScale create group view).

@bradgignac, without the 'inputChange' option the AutoScale group name field would be validated after every key stroke, which is not how it usually behaves elsewhere.

@bradgignac
racker member

Mentioned this in IRC, but I don't think we need to expand the public interface of this library to support both use cases if we change the extender to return a writeable computed observable.

  1. By default, everything just works. If we change an input or change the value programatically, validation is triggered.

  2. If we want to change when validation is triggered, we can pass an option to Knockout's value binding. Validation just works without any custom code.

  3. For other items like the help text, we can use Knockout's event binding.

IMO, this matches the approach used internally for Knockout and has been vetted. It also leans upon existing Knockout concepts - if you know how to use Knockout you can change when validation occurs.

@luizfar

👍 I will change it.

@willcumbie

👍

luizfar added some commits Feb 11, 2014
@luizfar luizfar Run observable validations when observable change.
* Also adds a configuration to allow the validation to run only on input
changes, if desired.
e3ddc01
@luizfar luizfar Remove 'validatesOn' configuration. 91fc25a
@luizfar

Talked on IRC with @bradgignac yesterday. Two things:

  • I removed the 'validatesOn' configuration, so we should rely on the 'event' binding for updating the UI without updating the observable value when required (such as AutoScale create view).

  • Refactoring the validators extenders to return a computed observable is tricky and probably would not work when we want to validate an already computed observable (for instance by using the validatesAfter extension). So we are keeping the current subscribe solution (which may not be as elegant but is also vetted).

@jpbochi

Let me understand one thing. This will be a breaking change. It will cause validators to run a different event now, right?

@trecenti trecenti commented on the diff Feb 12, 2014
src/ko-validation.js
@@ -126,19 +129,6 @@ ko.validation.registerValidator = function (name, validatorFactory) {
return validationElement;
}
- function bindEventListenerToRunValidation(element, observableToValidate) {
- var elementType, eventName;
-
- elementType = element.getAttribute('type');
@trecenti
trecenti Feb 12, 2014

what happens to validations on checkboxes? will it work as intended with the subscribe?

@jpbochi
jpbochi Feb 12, 2014

I'd hope an integration test to cover that.

@luizfar

@jpbochi, it will cause the validator to run whenever an observable changes, which includes the current behavior (when a change event happens on the input bound to the observable).
It would change how the AutoScale group name field on the create view behaves, but this PR avoids it: https://github.com/racker/reach/pull/9118

@luizfar luizfar merged commit 90b2243 into master Mar 12, 2014

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment