segmentio / validate-form

Easily validate a form element against a set of rules.
43 stars 6 forks source link

options, ability to validate on empty, async validation of entire form #9

Closed kc-dot-io closed 10 years ago

kc-dot-io commented 10 years ago

As per a brief exchange with @ianstormtaylor on twitter, I was looking for a way to have validate-form async validate all form fields when a user clicks submit.

The need for this behaviour was mostly emphasized by the assumption that empty inputs shouldn't be validated on blur. This was resulting in a user experience that required users to do a lot of back and forth to validate a long form as empty or missed inputs don't get validated on blur whatsoever.

I optimized a few things for these situations:

validate
  .set('validateEmpty', true)
    .field('input')
    ..is(...)

validate
  .set({ validateEmpty: true});
    .field('input')
    .is(...)

This should allow for more options to be added fairly easily.

form.validateAll(function(err, valid, fields) {
  if(!valid) return doError()
  else sendXHR();
});

I felt this approach would be best since it doesn't affect compatibility but provides the desired result as new functionality. There was one wait test that seems to be failing, but I believe it already was. Let me know if there is anything you'd like to see different.

Thanks for this repo, I really like it.

ianstormtaylor commented 10 years ago

oh weird. can you explain the empty inputs more? that should already be being solved by these two lines:

also, form.validate(fn) should be validating all the inputs. is that different from what you want?

glad you like the lib!

kc-dot-io commented 10 years ago

Interesting, maybe I'm adding behaviour you already have that I wasn't able to unlock for some reason. I didn't spend any time in either of the two locations you are pointing at, so it's entirely possible I missed something. I must say, I'm not fully sure at first glance what those lines do... I'll start tracking it in a minute.

Here is what I am referring to with the empty inputs - The assumption is that they should never be validated if empty && blur. My usability really begs for this behaviour, so I just made it over ridable.

The second concern was that when I was running form.validate(fn) the validation would run until the first input in the form that did not validate and then stop, resulting in one click, one error message displayed (below the first error it came across).

The purpose of adding form.validateAll(fn) was my way of getting it to run the entire form validation and return the complete state of the form, which results in one click, many error messages displayed. If this is already a feature, maybe I'm just not clear on how to set it up?

kc-dot-io commented 10 years ago

I should also note, in my use case I'm 100% using e.preventDefault() to suppress native submitting then firing off an XHR / WS with the data I want sent after doing random stuff. Perhaps this has an affect as I saw a few places you are watching submit events?

kc-dot-io commented 10 years ago

I read over both the two lines. I can verify in the first case even though I have used is('required') form inputs that were empty did not validate on blur, as is explicitly checked for here.

As for the second link, I think that this maybe addresses async validation of the rules on one input? I'm trying to add the ability to add async validation of all the fields and their rules... hence form.validateAll(fn)

ianstormtaylor commented 10 years ago

sorry for the slow response. gotcha on blur that makes sense. i think it might be nicer to make the option passed to on instead, and called strict. so it would become: .on('blur', { strict: true })

ianstormtaylor commented 10 years ago

an then as for the validate all, i think we'd want to move that logic to segmentio/validator instead to have a way to no exit on the first invalid one. visionmedia/batch calls the option throws: true which we could copy. and then ideally the api would be backwards compatible, and we'd just add an option of like .all or .multiple to this lib

would be open to that PR to validator and then the blur as a separate PR on this lib

does that all make sense?

kc-dot-io commented 10 years ago

Sure that makes sense. I'll try to carve out some time for it when it makes sense for me as I'll have to also re-tool our internal use case since we now implement this PR and I'm stretched a bit thing as it is on our product timeline.

Thanks for the feedback.