Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(input): use ValidityState for required state #7123

Closed
wants to merge 2 commits into from
Closed

feat(input): use ValidityState for required state #7123

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5af9454
feat(input): use ValidityState for required state
starr0stealer Apr 16, 2014
fe8a298
feat(input): use ValidityState for required state
starr0stealer Apr 16, 2014
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
7 changes: 6 additions & 1 deletion src/ng/directive/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,7 +1613,12 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* @returns {boolean} True if `value` is empty.
*/
this.$isEmpty = function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

changing $isEmpty is not the right approach because it gets overwritten a lot. Also, the patch needs tests

Copy link
Author

Choose a reason for hiding this comment

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

I chose to update NgModelController $isEmpty because I found that only checkboxInputType and ngListDirective had overrides to this method, which neither override (or input type) appeared to be lacking in validation logic. I originally implemented the logic in requiredDirective, but I felt that bloated the directive signature with the isObject(validity) check in order to support fallback for browsers that don't implement the HTML5 validation.

I am all ears on a more recommended placement of this change.

In regards of tests, I was unsure of any additional tests to be created as I seen 'required' already existed and passed still. Nor did I see any tests comparing the angular result to the ValidityState object

Copy link
Contributor

Choose a reason for hiding this comment

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

thats true, it is a stupidly hard thing to test. Mock element might be capable though.

However I don't think putting this in $isEmpty is the right approach. The check for a validity state only needs to happen once at compile time, and would look like this:

var validity = element.prop('validity');
if (!isObject(validity)) {
  validity = {
    valid: true
  };
}

This should be good enough, because then the test can simply be

if (validity.valueMissing || ctrl.$isEmpty(value))

Copy link
Author

Choose a reason for hiding this comment

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

Your right about the object check only once at compile time, I completely over looked that. Your recommended changes also fixes my concerns with method bloat. Hopefully I fully understand what you meant, because I updated requiredDirective which now looks like

var requiredDirective = function() {
  return {
    require: '?ngModel',
    link: function(scope, elm, attr, ctrl) {
      if (!ctrl) return;
      attr.required = true; // force truthy in case we are on non input element

      var validity = elm.prop('validity');
      if (!isObject(validity)) {
        validity = {
          valid: true
        };
      }

      var validator = function(value) {
        if (attr.required && (validity.valueMissing || ctrl.$isEmpty(value))) {
          ctrl.$setValidity('required', false);
          return;
        } else {
          ctrl.$setValidity('required', true);
          return value;
        }
      };

      ctrl.$formatters.push(validator);
      ctrl.$parsers.unshift(validator);

      attr.$observe('required', function() {
        validator(ctrl.$viewValue);
      });
    }
  };
};

return isUndefined(value) || value === '' || value === null || value !== value;
var validity = $element.prop('validity');
if (isObject(validity)) {
return validity.valueMissing;
} else {
return isUndefined(value) || value === '' || value === null || value !== value;
}
};

var parentForm = $element.inheritedData('$formController') || nullFormCtrl,
Expand Down