Skip to content

Introduce a new state for validation class. #257

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Introduce a new state for validation class. #257

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efa778c
Introduce a new state for validation class.
Jul 13, 2017
c806ca5
Fix code from feedback
Jul 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions src/formGenerator.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ div.vue-form-generator(v-if='schema != null')
validateAfterLoad: false,
validateAfterChanged: false,
validationErrorClass: "error",
validationSuccessClass: "",
validationSuccessClass: "success"
};
}
},
Expand All @@ -102,6 +102,7 @@ div.vue-form-generator(v-if='schema != null')

data () {
return {
touched: [],
errors: [] // Validation errors
};
},
Expand Down Expand Up @@ -165,22 +166,24 @@ div.vue-form-generator(v-if='schema != null')
// Get style classes of field
getFieldRowClasses(field) {
const hasErrors = this.fieldErrors(field).length > 0;
const wasTouched = this.fieldTouched(field);
let baseClasses = {
error: hasErrors,
error: hasErrors && wasTouched,
success: !hasErrors && wasTouched,
disabled: this.fieldDisabled(field),
readonly: this.fieldReadonly(field),
featured: this.fieldFeatured(field),
required: this.fieldRequired(field)
};

let {validationErrorClass, validationSuccessClass} = this.options;
if (validationErrorClass && validationSuccessClass) {
if (validationErrorClass && validationSuccessClass && wasTouched) {
if (hasErrors) {
baseClasses[validationErrorClass] = true;
baseClasses.error = false;
}
else {
} else {
baseClasses[validationSuccessClass] = true;
baseClasses.success = false;
}
}

Expand Down Expand Up @@ -283,6 +286,12 @@ div.vue-form-generator(v-if='schema != null')
onFieldValidated(res, errors, field) {
// Remove old errors for this field
this.errors = this.errors.filter(e => e.field != field.schema);
this.touched = this.touched.filter(e => e.field != field.schema);

this.touched.push({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why push touched fields every time a validation occurs?
Maybe have a logic

if( findTouchedField(field) == null)
  this.touched.push...

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is cleaned at every validation

field: field.schema,
touched: true
});

if (!res && errors && errors.length > 0) {
// Add errors with this field
Expand All @@ -301,10 +310,16 @@ div.vue-form-generator(v-if='schema != null')
// Validating the model properties
validate() {
this.clearValidationErrors();
this.touched.splice(0);

this.$children.forEach((child) => {
if (isFunction(child.validate))
{
this.touched.push({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering if it's not easier to have touched inside field data, initialize it with false and when you call this validation method you simply push to this array that value. This should happen only upon first validation

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, I started with this strategy (to have touched inside field data), but I didn't think about doing what you suggest.
The problem is that the style is decided getFieldRowClasses (in formGenerator.vue). This method argument is the field definition from the schema. This field definition is what is used to match "local" errors with "global" errors (fieldErrors for example).
I don't know why @icebob used this way to match things, but I can assume that there is a reason for it, a consequence I don't foresee right now (maybe for groups ?).
Anyway, I'm going to try another way to match fields for touched property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well the idea would be that you still have this touched array only that the touched property is kept inside the field just like errors https://github.com/icebob/vue-form-generator/blob/master/src/fields/abstractField.js#L27
You would keep touched inside abstractField data and change upon validation of the field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I understand, as I said, this was my first idea. But how do you access touched from formGenerator ?
I was going to add an index to the v-for loop ((field, index) in fields), but I noticed that groups will break this (I just noticed groups are not working how I assumed they would and this is another problem).
Anyway, changing an internal touched property is not the problem, accessing it from the parent (formGenerator) is the difficulty. How to match the children with the parent ?

Copy link
Collaborator

@cristijora cristijora Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just like it's done with errors. Iterate over children
Another, more cleaner way IMHO would be to have something like this:
Child field:

mounted(){
      this.$parent.addField(this)
    },
destroyed() {
   if (this.$el && this.$el.parentNode) {
         this.$el.parentNode.removeChild(this.$el);
    }
   this.$parent.removeField(this);
    },

Parent

addField(item) {
            const index = this.$slots.default.indexOf(item.$vnode);
            this.fieldData.splice(index, 0, item);
 },
 removeField(item) {
            const index = this.fieldData.indexOf(item);
            if (index > -1) {
                this.fieldData.splice(index, 1);
            }
 }

And you have all child info inside fieldData (including errors, touched property etc)
But that is probably out of scope now.

Copy link
Collaborator

@cristijora cristijora Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's a special data type/class which represents a component or a tree of elements in vue
https://github.com/vuejs/vue/blob/dev/src/core/vdom/vnode.js
It basically contains the element, tag, component data and many other things. Since slots return a list of $vnodes then you can do the extra check and see if the component you are trying to add exists in slots if slots are used. Since vfg uses component :is, then the filtering probably can be done based on $children maybe

Anyway that was an idea/draft which I copy pasted from one of my components so it has to be adapted to vfg implementation

Copy link
Member Author

@lionel-bijaoui lionel-bijaoui Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I feel so dumb right now 😅
I didn't now about this at all... My understanding of Vue is far from complete.

I like your solution a lot. Maybe I can introduce it only for this new state for now, and maybe move errors and other functionalities later if no problem arise ?
If I understand correctly, I need to change that:

addField(item) {
            const index = this.$slots.default.indexOf(item.$vnode);
            this.fieldData.splice(index, 0, item);
 }

into this:

addField(item) {
            const index = this.$children.default.indexOf(item.$vnode);
            this.fieldData.splice(index, 0, item);
 }

Copy link
Collaborator

Choose a reason for hiding this comment

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

addField(item) {
            const index = this.$children.findIndex(field=> field.$vnode == item.$vnode);
            this.fieldData.splice(index, 0, item);
 }

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok so your solution doesn't work.
When validateAfterLoad is true, the loop will trigger getFieldRowClasses before any field component initialization (created or mounted) and thus fieldData is empty, causing an error.
I think a solution like this would need a big rewrite of VFG internal.
I'm sorry but I cannot allow myself to lose any more time on this. I'm going to implement the fixes you suggested but I will keep this "dirty" for now.
Hope you understand, thank you for your help and feel free to do another PR with a better solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure no problem. Those were only suggestions, If they take too much time then any solution is ok

field: child.schema,
touched: true
});

let errors = child.validate(true);
errors.forEach((err) => {
this.errors.push({
Expand Down Expand Up @@ -342,6 +357,10 @@ div.vue-form-generator(v-if='schema != null')
return res.map(item => item.error);
},

fieldTouched(field) {
return this.touched.find(e=>e.field == field);
},

getFieldID(schema) {
const idPrefix = this.options && this.options.fieldIdPrefix ? this.options.fieldIdPrefix : "";
return slugifyFormID(schema, idPrefix);
Expand Down