search

contributte / live-form-validation

:no_entry: Nice client-side live form validation for Nette Forms.
https://contributte.org/packages/contributte/live-form-validation.html
BSD 3-Clause "New" or "Revised" License
59 stars 24 forks source link

Remove form's "hasError" property? #29

Open Robyer opened 7 years ago

Robyer commented 7 years ago

I just realized that since beginning is in code this this.setFormProperty(el.form, "hasError", true); call (in LiveForm.addError) and related checks.

It behaves like this: 1) When first error is added, it sets form.hasError 2) This form.hasError property is reset only when whole form is validated (when calling Nette.validateForm, e.g. on pressing submit) 3) When form.hasError is true, new errors aren't notified (by alert()) and elements with new errors aren't focused anymore. See Nette.addError.

I don't understand what is it good for and I'm thinking about removing that altogether.

Can somebody tell me if that code is important/useful for some reason?

Robyer commented 7 years ago

Do have e.g. @mcbmcb0 or @rolandtoth some idea?

mcbmcb0 commented 7 years ago

i'll have a look when i get some time - likely friday cheers

rolandtoth commented 7 years ago

No idea here :)

mcbmcb0 commented 7 years ago

yes i think your sequence of events in accurate - that property means only the first error triggered is focused or alerted.

i think it makes little practical difference for when live-validaiton is used. BUT:

I made a form with two elements with no-live-validation class. when both these elements have errors and when this.setFormProperty(el.form, "hasError", true) is commented out then when the form is submitted the user gets one alert for each element with an error. if you had six such 'no-live-validation' errors you'd get six sequential alerts. to me that's annoying. You can suppress this behaviour with showAllErrors:false; which would be technically more versatile but some users may find this an extra confusing config step. And with live-validation I like to see all the errors at once, not just one at a time after each submit. And the alerts provide so little useful information (eg:'required' with no form label to locate the error) that less alerts are better. or maybe rewrite the 'no-live-validation' alerts to actually be useful.

I say leave that line/property in - its doing no harm and making the code more usable.

Robyer commented 7 years ago

I say leave that line/property in - its doing no harm

It is doing harm as mentioned in your first line:

that property means only the first error triggered is focused or alerted.

It feels it is broken. When I was fixing issue #28 I felt it didn't work properly at all when inputs weren't focused after next error. However I agree that situation you describe with 'no-live-validation' is better with only single alert - but that could be solved on different place / in a better way.

So I'm thinking about: 1) Notify via alert() only first error - but only when validating the whole form (submitting the form). 2) For live-validation errors make focus and notify via alert() always work. NOTE: Perhaps alert() won't be called for live-validation errors anyway, because we don't attach any handlers to 'no-live'validation' inputs.

3) IN FUTURE: When validating the whole form, combine all errors into single alert(), similarly as netteForms.js does it now. And remove the "hasError" completely.

QUESTION: Is there a situation where alert() is called during live-validation? Or is it always called only when validating whole form?

mcbmcb0 commented 7 years ago

  1. I don't see any occurrence of alerts other than with errors on submit with no-live-validation.

  2. I don't really understand the problem with the current behaviour. When I blur out of an elements that has invalid data it won't focus on that element anyway as the blur occurs when I click/focus elsewhere. And as long as the error is highlighted (which it is) then I'm happy. I guess that may mean the code we are discussing is actually unnecessary (except when no-live-validation). It does focus on the first error when the submit is pressed - good. Hence - sure it could be improved but currently satisfactory in my view.

  3. I like your suggestions about compiling an alert message of all errors with no-live-validation. In my view in would be best with the corresponding labels alongside the errors which should be feasible given the label structure nette produces is predictable (using for...). That alert is very undeveloped - doesn't even alert LiveForm.options.messageErrorPrefix + message yet.

i appreciate your work on this project cheers Mike