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

Make v-form validation great again #1581

Merged
merged 9 commits into from
Sep 7, 2017
Merged

Conversation

KaelWD
Copy link
Member

@KaelWD KaelWD commented Sep 3, 2017

Form validation (<v-form v-model="valid">) is kinda useless right now:

  • It only works on immediate children, so nested components such as date pickers don't count
  • valid only changes when every single form field has been updated, whether they require validation or not. This makes optional fields impossible without having to call form.validate()
  • If a field is dynamically added to the form (with v-if or similar), it doesn't ever count towards validation, even if form.validate() is called
  • If a field is dynamically removed from the form and is invalid, the form will remain in an invalid state, even if form.reset() or form.validate() is called

This PR fixes all these issues, making validation of forms like this or this possible.

  • Deeply nested children will be included in the validation
  • valid will change after every update. form.validate() can still be called to check overall form validity and force validation errors to display on untouched invalid fields
  • A new boolean prop lazy-validation is added that only takes into account touched fields when determining overall validity (this is the same as it is currently) - so form.reset() will result in valid being true
  • Dynamically added fields will work the same way as static ones
  • Dynamically removed fields will be removed from consideration, so if an invalid field is removed from an otherwise valid form, valid should now be true

@KaelWD KaelWD changed the title [WIP] Do a deep search of VForm children to get inputs Do a deep search of VForm children to get inputs Sep 3, 2017
@KaelWD KaelWD changed the title Do a deep search of VForm children to get inputs [WIP] Make v-form validation great again Sep 3, 2017
 - data.inputs is now an array instead of an int
 - Dynamically added inputs contribute to form validity
 - Removed inputs no longer contribute to form validity
 - Now emits on every change, allowing optional fields
 - Removed redundant function calls
@KaelWD KaelWD changed the title [WIP] Make v-form validation great again Make v-form validation great again Sep 3, 2017
 - Validate runs through all inputs again instead of stopping at the first error
@KaelWD KaelWD mentioned this pull request Sep 6, 2017
@johnleider
Copy link
Member

Create a test corresponding to the changes and I'll finish my review and merge, thank you.

@johnleider johnleider self-assigned this Sep 7, 2017
@johnleider johnleider self-requested a review September 7, 2017 01:49
Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

}, false)

return !errors
return !this.inputs.filter(input => input.validate(true)).length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could/should be !this.inputs.every(input => input.validate(true))

Copy link
Member Author

@KaelWD KaelWD Sep 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that in 9a5063b, but it stops at the first invalid field. I can change it to ...length !== 0 to make it more clear what is happening.

Edit: or

const errors = this.inputs.filter ... .length
return !errors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also possibly use this.inputs.some(input => !input.validate(true))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.some has the same problem as .every.

The every method executes the provided callback function once for each element present in the array until it finds one where callback returns a falsy value. If such an element is found, the every method immediately returns false.

some() executes the callback function once for each element present in the array until it finds one where callback returns a truthy value (a value that becomes true when converted to a Boolean). If such an element is found, some() immediately returns true.

input.validate() must be called on every element so that validation errors are forced to display.

}

this.inputs.push(child)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove space

this.$vuetify.load(() => {
this.getInputs().forEach((child) => {
this.inputs += 1
this.$vuetify.load(() => this.watchInputs())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be this.$vuetify.load(this.watchInputs)

Copy link
Member Author

@KaelWD KaelWD Sep 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will pass the global Window object to watchInputs, instead of undefined.

Is .load() even needed? Nothing seems to change if I remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for SSR protection lol

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also valid point, no need to change this.

@azaars
Copy link
Contributor

azaars commented Sep 7, 2017

Brilliant!

Now I begin to think that external validators are second class. I believe this enhancement should work on v-stepper, that is by wrapping it with v-form, or perhaps even better if we have v-stepper built in with v-form, which can optionally be enabled with some props like use-form.

Looking forward for a quick merge!

@@ -78,11 +75,13 @@ export default {
})
},
validate () {
return !this.inputs.filter(input => input.validate(true)).length
return !this.inputs.filter(input => input.validate(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reasoning for this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks for pointing that out. That wasn't meant to be pushed.

 - Make validate() method more readable
@lock
Copy link

lock bot commented Apr 16, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please direct any non-bug questions to our Discord

@lock lock bot locked as resolved and limited conversation to collaborators Apr 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants