Use the proper semantics of an AngularJS control

[alfaproject]

This is obviously a massive breaking change because: 1) It makes ngModel mandatory (like any other AngularJS form control). 2) It changes the required semantics back to the same as any other AngularJS control in the sense that it's managed by AngularJS ngRequired directive instead of being micromanaged inside this control. At the moment, the required semantics are counter-intuitive and actually conflict a bit with the new invisible reCAPTCHA workflow.

I've changed the unit tests to match a traditional AngularJS control flow. Let me know what you think. If you don't agree with this, then we'll just have to keep our fork.

P.S.: In our build system, the HTML boolean attributes are optimised because we use HTML5, so if we put required="anything here" in our views, our minifier will change it to required because it's an HTML boolean attribute.

[coveralls]

Coverage increased (+1.1%) to 95.614% when pulling b7a1a4f1aa1e8153fbdf8605d77f487b50dc9f5b on xcaliber-tech:ng-model into 0e205bcebaf202b79a7cfe20e90e943016530331 on VividCortex:master.

[coveralls]

Coverage increased (+0.2%) to 94.737% when pulling b7a1a4f1aa1e8153fbdf8605d77f487b50dc9f5b on xcaliber-tech:ng-model into 0e205bcebaf202b79a7cfe20e90e943016530331 on VividCortex:master.

[coveralls]

Coverage increased (+1.1%) to 95.614% when pulling 397007548d05859f791bd28b155cc231e55200e5 on xcaliber-tech:ng-model into 0e205bcebaf202b79a7cfe20e90e943016530331 on VividCortex:master.

[coveralls]

Coverage increased (+0.2%) to 94.737% when pulling ef6e116f01930955faf5fae2e9d3070dd1b5e6d9 on xcaliber-tech:ng-model into 0e205bcebaf202b79a7cfe20e90e943016530331 on VividCortex:master.

[coveralls]

Coverage increased (+1.1%) to 95.614% when pulling ef6e116f01930955faf5fae2e9d3070dd1b5e6d9 on xcaliber-tech:ng-model into 0e205bcebaf202b79a7cfe20e90e943016530331 on VividCortex:master.

[TheSharpieOne]

It changes the required semantics back to the same as any other AngularJS control in the sense that it's managed by AngularJS ngRequired directive instead of being micromanaged inside this control. At the moment, the required semantics are counter-intuitive and actually conflict a bit with the new invisible reCAPTCHA workflow.

ngRequired only work on inputs, selects, and textareas. Removing it means you are removing that feature. While that feature doesn't make sense with the invisible reCaptcha, it does make sense for the checkbox reCaptcha. Without the required (opt-in or opt-out) and the $setValidity call, angular has no way of knowing that the reCaptcha is invalid or not and users cannot rely on the form validity anymore.

To clarify, you are not changing the required semantics back to the same as any other AngularJS control, you are removing the feature completely from this library and the users will no longer be able to make the reCaptcha field required. This is not a bad thing. Again, with invisible reCaptcha, required is not needed.

Also, why make an optional field mandatory? What is the benefit? Right now if someone wants to provide ngModel, they can. If they don't want to for whatever reason, they don't have to. Unless there is a reason/feature which requires ngModel to always be there, their should be no reason to require it. Don't get me wrong, I always use ngModel as if it was required. I am the person who added the ability to use ngModel with current version of this library (after the rewrite to support reCaptcha v2). It was mostly needed to tie into angular's validation, even then it wasn't necessary to make it required since some people would not need the validation. Now that validation is being removed by this PR, ngModel becomes required... doesn't make as much sense.

[alfaproject]

Actually, I just realised I should have added tests for ngRequired, so yes you are right. I killed that by accident, doh. I'll add tests and bring form validity back. (as you said, we don't really need it for invisible, so it slipped through the cracks, sorry)

Regarding forcing ngModel, it's basically best practice. This control is intended to be used in a form and I can't think of any other use case. I understand your point, but I think we should force people to be better citizens, or maybe I'm wrong. :F

Not forcing anything btw. Just stating my reasoning, and getting feedback, so thank you for that. (:

[coveralls]

Coverage increased (+1.1%) to 95.614% when pulling 0f79f6d31b913afc9f838491c347dac06fe84af7 on xcaliber-tech:ng-model into 0e205bcebaf202b79a7cfe20e90e943016530331 on VividCortex:master.

[alfaproject]

@TheSharpieOne I've added more unit tests and assertions for the native ngRequired validation.

From the ngRequired documentation: When developing custom controls, $isEmpty() can be overwritten to account for a $viewValue that is not string-based.

In our case, our $viewValue is string based, so we don't need to override $isEmpty, and the validation works fine. This obviously works because I'm forcing ngModel to be used in this PR, and I still strongly believe that's the right way to make a custom form control, so we can take advantage of AngularJS native functionality.

Let me know what you think, please. (:

[mtrias]

Looks good @alfaproject. Thank you! Could you please remove the changes to the files that are auto-generated? We usually only merge changes to source files.

Also, should we update the README file as part of this change ?

[alfaproject]

@mtrias I will do the README changes and any other changes needed to the demo files, but do you mind if I keep the autogenerated files except for the minified one? The reason is that we are using my forked branch on production already, so if I remove those, I'll break our applications on the next CI build. ):

[TheSharpieOne]

You can make a new branch from your ng-model branch and delete the generated files from the new branch and make a new PR with the new branch.

[coveralls]

Coverage increased (+0.2%) to 94.737% when pulling 5a85ccb2a1e314b61e8c131f02443077535fdaa1 on xcaliber-tech:ng-model into 18f7372e0ccd9a51c9c39a55e7e7a1c4616787e0 on VividCortex:master.