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

Add new type of ngrxUpdateOn #118

Closed
Mr-Eluzive opened this issue Oct 17, 2018 · 4 comments
Closed

Add new type of ngrxUpdateOn #118

Mr-Eluzive opened this issue Oct 17, 2018 · 4 comments

Comments

@Mr-Eluzive
Copy link
Contributor

Mr-Eluzive commented Oct 17, 2018

I use this repo in quite big project. We also use ng-boostrap's typeahead. And here I've faced some issue. We update store in custom way by using typeahead's hooks.

<input *ngIf="values"
  #instance="ngbTypeahead"
  type="text"
  class="form-control"
  ngrxUpdateOn="blur"
  [id]="formId + '.' + controlName"
  [resultFormatter]="resultFormatter"
  [ngbTypeahead]="search.bind(this)"
  [inputFormatter]="inputFormatter"
  [ngrxFormControlState]="(parentFormControl$ | async)"
  (selectItem)="selectItem($event)"
  (blur)="updateValueOnBlur($event)"
  (focus)="onFocus($event)"
  (click)="onClick($event)"
/>
selectItem($event) {
  const controlId = `${this.formId}.${this.controlName}`;
  const selectedKey = _.get($event.item, this.keyPath);
  this.store.dispatch(new SetValueAction(controlId, selectedKey));
  this.valueSelected.emit($event.item);
}

updateValueOnBlur($event) {
    const {value} = $event.target;
    const sameValue = this.values.find(x => _.get(x, this.valuePath).toLowerCase() === value.toLowerCase());
    if (sameValue) {
        this.selectItem({
            item: sameValue, preventDefault: () => {},
        });
    }
}

Updating store by ngrx-forms creates an issue for us (sends whole not serialized object) but we still want to have some of your cool features like validation etc.
Maybe we should add another ngrxUpdateOn type like never which actually doesn't send value to store but dispatch MarkAsDirty action to run validation etc.

What do you think about this idea?

EDIT. I've even created PR already, because it's quite necessary for us.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Oct 17, 2018

Can you please elaborate a bit more on what the exact issue is? I don't get what you mean by "sends whole not serialized object".

There may also an alternative to what you suggest: ngrx-forms subscribes to the input event for dispatching the SetValueAction. Therefore, if you add your own handler for this and stop the event's propagation the action should not be dispatched. However, there is one tricky bit and that is the order in which the event handlers are called. I am not sure if this is specified somewhere, but in my experience the handlers are added in the order they appear in the HTML. Therefore, could you please check if this workaround works for you?

<input *ngIf="values"
  #instance="ngbTypeahead"
  type="text"
  class="form-control"
  ngrxUpdateOn="blur"
  [id]="formId + '.' + controlName"
  [resultFormatter]="resultFormatter"
  [ngbTypeahead]="search.bind(this)"
  [inputFormatter]="inputFormatter"
  (input)="$event.stopPropagation()"
  [ngrxFormControlState]="(parentFormControl$ | async)"
  (selectItem)="selectItem($event)"
  (blur)="updateValueOnBlur($event)"
  (focus)="onFocus($event)"
  (click)="onClick($event)"
/>

@Mr-Eluzive
Copy link
Contributor Author

I mean we do have these updateValueOnBlur and selectItems methods which are responsible for updating our store by key of the selected object from typeahead. And it works. But when this input lose focus there is additional update to store from your directive (ngrxUpdateOn="blur") and it doesn't work like we want. Actually we get error:

ERROR Error: Form control states only support undefined, null, string, number, and boolean values; got {"value2":"PL",[ ... ],"key":"616"} of type "object"

(input)="$event.stopPropagation()" unfortunately doesn't work (add stopPropagation to blur method also) but solution with never does.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Oct 18, 2018

Ah, I understand now. The typeahead control value accessor is setting the whole object as the value instead of just the key. In this case I have two other suggested workarounds for you while we are working on the PR.

First of all, looking at the code from bootstrap I see that they have support for preventing the typeahead to report the value change. So could you please just try adding $event.preventDefault() to your selectItem handler?

Secondly, this scenario calls for a value converter. To be honest I am not sure how it works properly without it. Looking at the code for the typeahead again the inputFormatter is called whenever the control's value changes. Since you are putting the key in the control state the value handed to the inputFormatter will not be the object but just the key. Are you already handling this by projecting the key to some display label for the selected item?

Usually with ngrx-forms there is no need for things like inputFormatter. You can just provide a value converter that inside the convertViewToStateValue selects the key property from the object that is being set by the typeahead and inside the convertStateToViewValue just does whatever your inputFormatter does. Could you please try this?

@MrWolfZ
Copy link
Owner

MrWolfZ commented Oct 21, 2018

I have just released version 3.1.0 which contains the enhancement from your PR.

@MrWolfZ MrWolfZ closed this as completed Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants