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

Do not validate immediately #97

Open
lonix1 opened this issue Mar 2, 2024 · 8 comments
Open

Do not validate immediately #97

lonix1 opened this issue Mar 2, 2024 · 8 comments

Comments

@lonix1
Copy link
Contributor

lonix1 commented Mar 2, 2024

In the original jQuery plugin it was possible to control when validation was activated. By default it would occur on each field's blur (demo).

This library performs validation immediately, i.e. as soon as the user starts typing in a field. That is not standard validation behaviour and leads to poor UX - the user is distracted by validation errors even before submitting the form.

Repro: I wanted to ensure it's not "just me", so:

  • I cloned the repo and ran the sample app
  • In the "Form 1" sample, type anything into the "Id (42)" field
  • Even before submitting, the validation errors are shown

I can identify these modes of activation:

  • current behaviour: validation is activated (for each field) immediately as the user types into a field
  • on form submit: validation is activated (for all fields) only when the user attempts to submit the form
  • on field blur (original plugin's behaviour): validation is activated (for each field) only after a field's first blur
  • on field blur with change: validation is activated (for each field) only after a field's blur and where the value was actually changed (to protect against user simply tabbing/clicking through fields without actually changing them)

Is any of this currently possible?

@lonix1 lonix1 changed the title Do not validate before submit Do not validate immediately Mar 3, 2024
@dahlbyk
Copy link
Collaborator

dahlbyk commented Mar 4, 2024

By default it would occur on each field's blur

I believe the intent is still to be as close as we can to a drop-in replacement, so aligning our default with the old library seems reasonable.

I can't think of any reason we wouldn't always want to capture fields' initial value so we can defer validation until that value has changed. We'd still always validate on submit, of course.

Is any of this currently possible

You can specify data-val-event="blur" per field to override:

let validateEvent = input.dataset.valEvent;
if (validateEvent) {
input.addEventListener(validateEvent, cb);
}
else {
let eventType = input instanceof HTMLSelectElement ? 'change' : 'input';
input.addEventListener(eventType, cb);
}

We can take a closer look at how the old library can be customized, but in the short term we could pretty easily support bootstrapping with { fieldValidationEvent: 'blur' } override for all fields.

Thoughts?

@lonix1
Copy link
Contributor Author

lonix1 commented Mar 5, 2024

Thanks!

capture fields' initial value so we can defer validation until that value has changed

Yes I did something similar - I saved the initial value to a data attribute, e.g. data-original="foo" for each field, on document ready.

You can specify data-val-event="blur" per field to override

I was able to delete so many kludges and workarounds because of that feature. Thanks! I've lodged a docs PR to expose it.

bootstrapping with { fieldValidationEvent: 'blur' } override for all fields

Although that would be nice, but data-val-event="blur" is probably good enough for now.

But deferring validating until the field is actually dirty would be a good addition. Without that, some ugly workarounds are still necessary.

I haven't written TypeScript in years, so I can't help there, but maybe someone will stumble on this issue for the same reason and help out. Just ping me to get involved and help with testing / documenting, etc.

This was referenced Mar 5, 2024
@haacked
Copy link
Owner

haacked commented May 6, 2024

Was this addressed in #110?

@dahlbyk
Copy link
Collaborator

dahlbyk commented May 6, 2024

Was this addressed in #110?

I don't think so

@andkorsh
Copy link

andkorsh commented Aug 8, 2024

Would like to add my two cents - ideally the logic would be a little bit more complicated than switching validation event to blur. I think it shouldn't do any validation while the user typing for the first time (i.e. until the first blur or submit) but once the validation had failed and user focused back into the input, we should switch back to "input" event to remove the error as soon as the data is valid.

upd: I was playing with the code there a little bit trying to implement the desired behavior and noticed that data-val-event value isn't really used - looks like parenthesis are missing in this line:

const validateEvent = input.dataset.valEvent || input instanceof HTMLSelectElement ? 'change' : 'input';
So whenever there's a data-val-event attribute present, no matter it's value, "change" event will be used. Not sure if it is intentional though.

@lonix1
Copy link
Contributor Author

lonix1 commented Aug 9, 2024

The original plugin's behaviour is to validate on blur, so that should be the default given that this library is intended to be a drop-in replacement.

That said, I suspect we're describing the same thing?

@andkorsh
Copy link

andkorsh commented Aug 9, 2024

@lonix1 I've created a PR that adds the new behavior for "delayed validation" using a new option like this:

   new ValidationService().bootstrap({ delayedValidation: true })

The default behavior is unchanged and will continue to be a drop-in replacement.

Apart of that, I found a bug with data-val-event that I have described above, which I have not fixed in my PR on purpose, because it changes the way data-val-event works. I can easily fix it, but it will change the behavior for people using this attribute. As it was suggested in the comments above, some may have specified data-val-event="blur", but due to a bug, when this attribute specified, it in fact uses "change" event, not "blur".

Although delayedValidation option might be the most desirable, let me know if I should fix the bug with data-val-event and I can do that in the same PR.

andkorsh pushed a commit to andkorsh/aspnet-client-validation that referenced this issue Aug 9, 2024
@lonix1
Copy link
Contributor Author

lonix1 commented Aug 10, 2024

I didn't realise you put it behind a new option, in which case it's all good. I suppose Keith / Phil should decide on the rest.

At some point we should also discuss the default behaviour as documented above.

andkorsh pushed a commit to andkorsh/aspnet-client-validation that referenced this issue Aug 15, 2024
…nts. Fixed a bug with data-val-event not being respected
haacked added a commit that referenced this issue Aug 16, 2024
#97 Implemented delayed validation by introducing delayedValidation o…
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

4 participants