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

#113 removing directive scope dependency #162

Merged
merged 4 commits into from
Dec 4, 2015
Merged

#113 removing directive scope dependency #162

merged 4 commits into from
Dec 4, 2015

Conversation

Nazanin1369
Copy link
Collaborator

Hi all,

Here is an initial commit removing the isolated scope from validator directive.

@hueitan
Copy link
Owner

hueitan commented Nov 30, 2015

cool @Nazanin1369 Does it really fix the problem 113 ? or you can tell us more about this fix.

It's looks perfect to isolate the scope.

@Nazanin1369
Copy link
Collaborator Author

This commit removes the isolated scope from the directive so we can have
other directives work with validator as AngularUI datepicker or anyother. I
will put up an example page tonight.
On Nov 29, 2015 6:05 PM, "Huei Tan" [email protected] wrote:

cool @Nazanin1369 https://github.com/Nazanin1369 Does it really fix the
problem 113 ? or you can tell us more about this fix.


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

@@ -288,7 +289,7 @@
*/
if (attrs.validMethod === 'blur') {
element.bind('blur', function() {
var value = ctrl.$viewValue;
var value = scope.$eval(attrs.ngModel);
Copy link
Owner

Choose a reason for hiding this comment

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

What's the difference between ctrl.$viewValue and scope.$eval(attrs.ngModel) in our model.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is the same but since we have ng-model defined as our directive attr I
think it is better to use that rather than controller view value. Then what
would be the usage of ngModel?
On Nov 29, 2015 9:51 PM, "Huei Tan" [email protected] wrote:

In src/validator.directive.js
#162 (comment)
:

@@ -288,7 +289,7 @@
*/
if (attrs.validMethod === 'blur') {
element.bind('blur', function() {

  •        var value = ctrl.$viewValue;
    
  •        var value = scope.$eval(attrs.ngModel);
    

What's the difference between ctrl.$viewValue and
scope.$eval(attrs.ngModel) in our model.


Reply to this email directly or view it on GitHub
https://github.com/huei90/angular-validation/pull/162/files#r46110404.

Copy link
Owner

Choose a reason for hiding this comment

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

looks good, but not sure if it exists any minor bug. (test cases look fine)

@hueitan
Copy link
Owner

hueitan commented Nov 30, 2015

Looks good 👍 Thanks @Nazanin1369

I will merge after you commit the example.

@hueitan
Copy link
Owner

hueitan commented Nov 30, 2015

Better if we can add test case for this isolate.

@Nazanin1369
Copy link
Collaborator Author

Sure. This is an initial commit. Will put up example and test case up
tomorrow.
On Nov 29, 2015 10:14 PM, "Huei Tan" [email protected] wrote:

Better if we can add test case for this isolate.


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

@hueitan
Copy link
Owner

hueitan commented Dec 3, 2015

@Nazanin1369 Can't wait to see your update 😄

@Nazanin1369
Copy link
Collaborator Author

I just put up a final commit. I have actually removed the child scope and added a Bootstrap datepicker demo to the index.html page. The ui-bootstrap datepicker is working fine with our validator directive now.

@Nazanin1369 Nazanin1369 changed the title #113 removed isolated scope #113 removing directive scope dependency Dec 3, 2015
@hueitan
Copy link
Owner

hueitan commented Dec 3, 2015

What is the main.html page?
also we may need some unit test cases for this change, or I will do it later to make sure this works.

hueitan pushed a commit that referenced this pull request Dec 4, 2015
[Fix #113] removing directive scope dependency
@hueitan hueitan merged commit 1171c4a into hueitan:master Dec 4, 2015
@woodensail
Copy link

It seems @Nazanin1369 use the messageId to prevent confusion between datepicker element and validate element in the demo page.
Lack of messageId can lead to mistakes, and it's hard to check because this is not mentioned in the document.

Finally i configed getMsgElement like this, to solve it.

$validation.getMsgElement = function (element) {
  return element.nextAll('.validation-tip');
};

So, do you think you should consider adding some instructions about third party plug-in and the messageId, or just change the default getMsgElement Function?

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

Successfully merging this pull request may close these issues.

3 participants