Support validation group #175

lvarayut · 2015-12-19T19:31:03Z

I added a validation-group attribute that could be used to group many elements into a group. The group will be considered as valid if and only if one of them is valid. Otherwise, the group will be marked as invalid. A valid/invalid message will be placed inside an element that contains an id attribute with the same name as provided to the directive validation-group.

<label>Validation group</label>
<!-- Group both of these elements inside the contact group -->
<input type="text" name="email" ng-model="email" validator="required" validation-group="contact"> 
 <input type="number" name="telephone" ng-model="telephone" validator="number" validation-group="contact">
 <!-- The message will be placed in side the span element -->
 <span id="contact"></span>

Closed #40

hueitan · 2015-12-23T08:55:05Z

test/unit/directivesSpec.js

+      messageElem = angular.element(element[0].querySelector('#contact > p'));
+      expect(messageElem.hasClass('validation-invalid')).toBe(true);
+    });
+  });


@lvarayut Do you want to create a new validationGroupSpec.js for this test?

Since we have too many test cases in this file.

That's what I was thinking at the first time. However, I decided to put it in the same file because the validation-group is in the directive file. I'm afraid that we would get confused afterward. What do you think?

Me too, I'm thinking about the same thing before. As you can see the current folder of the test, I have combined all the directive spec in different files.

I agree with you that we may get confused afterward, but without making it clear, the test file will harder to modify and fix.

I suggest we create a new test file if given a new test case, for the previous old one we can keep them originally until we enhance them and move them to the new file.

What do you see?

You're right. Let's do it!

hueitan · 2015-12-23T08:58:41Z

Is this possible to validate group?

for example we validate the Form using

$validationProvider.validate(Form);

validate single input

$validationProvider.validate(Form.required);

I wish we can have the opportunity to validate group

$validationProvider.validate(checkboxGroup);

hueitan · 2015-12-23T09:00:13Z

For every validation-group, there is only one output message, am I right?

hueitan · 2015-12-23T09:11:58Z

dist/angular-validation.js

@@ -379,7 +379,7 @@
      var validCallback = $parse('success');
      var messageElem;

-      if (attrs.messageId) messageElem = angular.element(document.querySelector('#' + attrs.messageId));
+      if (attrs.messageId || attrs.validationGroup) messageElem = angular.element(document.querySelector('#' + (attrs.messageId || attrs.validationGroup)));


What about define

var validationGroup = attrs.validationGroup

so that we use validationGroup instead of repeatly calling attrs.validationGroup within the function.

That's a great advice! Same for attrs.messageId. I'll fix both of them.

Thank you. Same for other attributes :)

lvarayut · 2015-12-23T12:06:00Z

$validationProvider.validate(Form);
$validationProvider.validate(Form.required);

I haven't tested it but it should work in both of them because, in the code, I just verified that in case of any elements of the group error, it will show an error message. Otherwise, it'll be valid. All the "validation" process still remains the same as before. I'll write some more unit tests!

I wish we can have the opportunity to validate group
$validationProvider.validate(checkboxGroup);

I'll take a look at this case.

hueitan · 2015-12-23T13:50:12Z

Ok, I will test that too :)

hueitan · 2015-12-24T01:58:58Z

test/unit/validationGroupSpec.js

+      expect(messageElem.hasClass('validation-invalid')).toBe(true);
+    });
+  });
+});


Thanks @lvarayut

hueitan · 2015-12-24T02:10:43Z

@lvarayut I have gone through your code

so this attribute validation-group should only use in the same Form? Is it not able to group in different Form?

different Form:

<form name="Form1"></form>

<form name="Form2"></form>

We should clarify this condition in the API document, because the term "group" is easily misunderstood.

lvarayut · 2015-12-24T03:08:59Z

We should clarify this condition in the API document, because the term "group" is easily misunderstood.

Ok. I'll say it in the API.md. I think that most of people will use it in the same form because when we wanted to group somethings, they would normally be placed near each other, for example, checkboxes, contact fields, etc. It's not quite logic to group elements between different forms. However, I'll put the clear word in our document to prevent the rare situation.

hueitan · 2015-12-24T03:21:11Z

There are two more things to do

use attribute as a variable (don't repeatedly calling attrs.something)
given document about usage of validation-group

Thank you @lvarayut after these ready to merge 😄

🍻

lvarayut · 2015-12-24T03:24:23Z

Thanks @huei90 for your great review. I'll do as your suggestions and also the following:

Comment the code!!
Write more tests about provider.validate();

hueitan · 2015-12-24T03:27:28Z

Thank you @lvarayut @Nazanin1369

Tests are now more important as our project is getting bigger and having a lot of features, so we should take the test cases very seriously and carefully in our future commit.

☀️

- Add the note in order to clearly state that the validation-group can only be used for elements placed in the same form tag

process

hueitan · 2015-12-27T14:06:46Z

Seems everything is ready, I'm going to merge it. 👍 @lvarayut

lvarayut · 2015-12-27T14:20:29Z

Thanks @huei :)

On Sunday, December 27, 2015, Huei Tan [email protected] wrote:

Seems everything is ready, I'm going to merge it. [image: 👍] @lvarayut
https://github.com/lvarayut

—
Reply to this email directly or view it on GitHub
#175 (comment)
.

Support validation group

juw177 · 2016-02-02T04:54:44Z

Hi, am I missing something here? All this does is hide the error message but the form is still invalid?

checkValid does not take validation groups into account, so how can this work? You can see this on the demo page with the 2 check boxes. Or is this intended behaviour?

juw177 · 2016-02-02T05:32:06Z

Also, this should only be targeting the required expression. eg, If a form field is an email, it makes no sense to allow the user to submit whatever they want just because another element in the group is valid?

hueitan · 2016-02-02T06:00:03Z

@lvarayut

lvarayut · 2016-02-03T14:00:31Z

@juw177 Well. It depends on the logic of your application. If you need only one field to be filled out by a user, as shown in the example, it totally makes sense and it's the purpose of using validation-group. The way it works is pretty straight forward as mentioned in the API.md:

The group will be considered as valid if and only if one of them is valid. Otherwise, the group will be marked as invalid.

It didn't hide any error message. If one of the elements is valid, the entire form is still valid.

juw177 · 2016-02-04T00:08:52Z

@lvarayut Thanks for that explanation. However, I am not seeing this behaviour of the entire form being valid when I go to the example page here:

http://huei90.github.io/angular-validation/#examples

If you go to the form with Checkbox 1 and Checkbox 2, both with validation-group="checkBlur", the form still shows checkValid=false if you only check one of the boxes. Only the error message gets hidden.

lvarayut · 2016-02-04T02:11:16Z

You're right. Thanks for reporting. I'm working on it. @juw177, if you could come up with any good idea to handle it, please let me know. Or if you could send a PR, that would be really welcomed.

hueitan · 2016-02-17T03:54:00Z

PRs are always welcomed. 🙏

lvarayut added 3 commits December 20, 2015 02:23

Add validation-group attribute

38dcc3a

Add unit tests for validation-group

c75b099

Update API.md to include validation-group directive

f91cef0

hueitan reviewed Dec 23, 2015
View reviewed changes

Separate validation-group tests to its own validationGroupSpec.js

ba518fd

hueitan reviewed Dec 24, 2015
View reviewed changes

test/unit/validationGroupSpec.js

expect(messageElem.hasClass('validation-invalid')).toBe(true);

});

});

});

Copy link

Owner

hueitan Dec 24, 2015

Choose a reason for hiding this comment

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

Thanks @lvarayut

lvarayut added 3 commits December 25, 2015 23:05

Add a caution note of using validation-group in API.md

5a992b9

- Add the note in order to clearly state that the validation-group can only be used for elements placed in the same form tag

Separate attrs properties into variables and Comment validation-group

4462bb1

process

Add validation-group unit tests using provider

4862114

hueitan pushed a commit that referenced this pull request Dec 27, 2015

Merge pull request #175 from lvarayut/validation-group

df9f4d4

Support validation group

hueitan merged commit df9f4d4 into hueitan:master Dec 27, 2015

hueitan mentioned this pull request Dec 28, 2015

Ticket and Enhancement #174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support validation group #175

Support validation group #175

lvarayut commented Dec 19, 2015

hueitan Dec 23, 2015

lvarayut Dec 23, 2015

hueitan Dec 23, 2015

lvarayut Dec 23, 2015

hueitan commented Dec 23, 2015

hueitan commented Dec 23, 2015

hueitan Dec 23, 2015

lvarayut Dec 23, 2015

hueitan Dec 23, 2015

lvarayut commented Dec 23, 2015

hueitan commented Dec 23, 2015

hueitan Dec 24, 2015

hueitan commented Dec 24, 2015

lvarayut commented Dec 24, 2015

hueitan commented Dec 24, 2015

lvarayut commented Dec 24, 2015

hueitan commented Dec 24, 2015

hueitan commented Dec 27, 2015

lvarayut commented Dec 27, 2015

juw177 commented Feb 2, 2016

juw177 commented Feb 2, 2016

hueitan commented Feb 2, 2016

lvarayut commented Feb 3, 2016

juw177 commented Feb 4, 2016

lvarayut commented Feb 4, 2016

hueitan commented Feb 17, 2016

Support validation group #175

Support validation group #175

Conversation

lvarayut commented Dec 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hueitan commented Dec 23, 2015

hueitan commented Dec 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvarayut commented Dec 23, 2015

hueitan commented Dec 23, 2015

Choose a reason for hiding this comment

hueitan commented Dec 24, 2015

lvarayut commented Dec 24, 2015

hueitan commented Dec 24, 2015

lvarayut commented Dec 24, 2015

hueitan commented Dec 24, 2015

hueitan commented Dec 27, 2015

lvarayut commented Dec 27, 2015

juw177 commented Feb 2, 2016

juw177 commented Feb 2, 2016

hueitan commented Feb 2, 2016

lvarayut commented Feb 3, 2016

juw177 commented Feb 4, 2016

lvarayut commented Feb 4, 2016

hueitan commented Feb 17, 2016