xmaestro / angular2-recaptcha

Angular 2 : Typescript component for Google reCaptcha
ISC License
79 stars 30 forks source link

ControlValueAccessor interface? #34

Closed achimha closed 7 years ago

achimha commented 8 years ago

Would it make sense to have the component implement the ControlValueAccessor interface and behave like a form control? Then we could get the captcha response as value from the form and assign it the required attribute.

What do you think?

xmaestro commented 8 years ago

Sounds like a good idea. :+1:

xmaestro commented 7 years ago

@achimha Any update on this?

budkin76 commented 7 years ago

+1 for form validation integration.

achimha commented 7 years ago

I've created a PR: https://github.com/xmaestro/angular2-recaptcha/pull/51

Needs some more testing before merging.

xmaestro commented 7 years ago

@achimha I think i made a premature merge. When i tested it did not give me value for ngModel binding and also change event does not trigger.

xmaestro commented 7 years ago

@achimha I tested it again and ngModel seems to work but change event does not work. But I was wondering whether we really need change and blur events. What do you think? I'm ready to publish if we only need ngModel.

xmaestro commented 7 years ago

@achimha I think these changes are good to go. Do you?

BeheadedKamikaze commented 7 years ago

I didn't get a chance to try out this update yet (will have a look when I get home) but the implementation of the interface looks correct to me

xmaestro commented 7 years ago

@BeheadedKamikaze I'll be waiting for your feedback then ;)

BeheadedKamikaze commented 7 years ago

It works fine (I can bind to the value and the form only becomes valid when the box is selected).

The only issue is if you call reset() then it doesn't invalidate the ngForm.

achimha commented 7 years ago

Good point @BeheadedKamikaze. Could you give this a try?

public reset() {
    if (this.widgetId === null)
        return;
    // noinspection TypeScriptUnresolvedVariable
    (<any>window).grecaptcha.reset(this.widgetId);
    this.onChange(null);
}

I wonder if undefined or null would be the right value.

BeheadedKamikaze commented 7 years ago

Yeah @achimha that was my thought as well. I just tried and it works - the form becomes invalid as soon as it expires. :+1:

I would go with null personally.

xmaestro commented 7 years ago

Ok. Great. I'll create a PR shortly.

BeheadedKamikaze commented 7 years ago

Cool.

Also just a quick note to anyone looking at binding this to an ngModel, for validation to work you must also set a name attribute or formControlName if you are using dynamic forms.

<re-captcha
  [site_key]="RECAPTCHA_SITEKEY"
  [(ngModel)]="reCaptchaToken"
  name="reCaptcha"
  required
></re-captcha>
xmaestro commented 7 years ago

Does the PR #59 look right?

BeheadedKamikaze commented 7 years ago

Yes looks great - thanks! :rocket:

xmaestro commented 7 years ago

Merged PR #59. Closing this. Let's create a new one in case of issues.