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

$setValidity should not be used with a blank error key. #16

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

idpaterson
Copy link
Contributor

This can unintentionally make the field required; if the text field is cleared while displaying an invalid phone number, the '' error key will remain invalid.

To test:

  1. Begin with a non-required international-phone-number field
  2. Type an invalid phone number such as +1 555 555 5555
  3. Delete the phone number leaving a blank field (or just the + prompt)

The field will have a validation error, $error: {"international-phone-number":false,"":true} because the blank validation key is still invalid. I believe that b0a4fea where this blank error key was introduced was attempting to resolve the problem fixed in #15.

@@ -73,7 +73,6 @@ angular.module("internationalPhoneNumber", []).directive 'internationalPhoneNumb
if value
validity = element.intlTelInput("isValidNumber")
ctrl.$setValidity 'international-phone-number', validity
ctrl.$setValidity '', validity
else
value = ''
Copy link
Owner

Choose a reason for hiding this comment

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

Today I read into the docs regarding this matter. I found out that this is not the best way to do validation checking. Consider this:

ctrl.$validators.internationalPhoneNumber = function(value) {
  return value && element.intlTelInput("isValidNumber");
};

This way it seems like a clean separation between parses (which prepare value for validation checking and eventually writing into model) and validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed better to use the new $validators pipeline, but unfortunately that would bump your minimum Angular version from 1.2 to 1.3. Would you consider merging this and then if the $validators approach is desired, add it as a breaking change in a 1.0.0 or 0.1.0 version?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, no problem. Please submit a pull request, I'll merge it as one of 0.0.*. Then I'll move the solution with $validators to 0.1.0 as you suggested

Validation now checks for the existence of ctrl.$validators, defaulting to the Angular 1.2 compatible ctrl.$parsers pipeline when necessary. Validation logic reverted to state as of version 0.0.4.
This can unintentionally make the field required; if the text field is cleared while displaying an invalid phone number, the '' error key will remain invalid.
We noticed that in some cases after clearing the field to type a new number the ng-invalid class stuck around despite ng-valid-international-phone-number. This fixes what may be due to an inconsistency in ngModelController after deleting the key from $error.
@idpaterson idpaterson force-pushed the pull-requests/remove-blank-error-key branch from f27fabc to 5ea6581 Compare April 21, 2015 13:43
@idpaterson
Copy link
Contributor Author

@mareczek since yesterday's 0.0.6 introduces $validators which breaks the module for Angular 1.2 users, I have updated this pull request against the current master so that it will once again allow changes to be merged after your changes latest commits.

I think there was some confusion since this pull request was pending and ready to merge for some time before that change was made and your comment was on the commit within the pull request rather than on the pull request itself.

After these changes, the module checks whether ctrl.$validators is defined to switch between Angular 1.2 compatible and 1.3 compatible validation. It would be a shame to drop Angular 1.2 support for a trivial issue like that. Most importantly, any Angular 1.2 users who upgraded to 0.0.6 would have noticed that the directive was not functioning due to assigning to the undefined ctrl.$validators.

Finally, the change in #15 has been included since #15 would otherwise have merge conflicts. Please consider reviewing other pull requests, then building the distribution scripts with a 0.0.8 tag to fix the Angular 1.2 compatibility issue.

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.

2 participants