feat(combo, simple-combo): filter out non-existent items from selection - master

[georgianastasov]

Closes #14732

Additional information (check all that apply):

• Bug fix
• New functionality
• Documentation
• Demos
• CI/CD

Checklist:

• All relevant tags have been applied to this PR
• This PR includes unit tests covering all the new code (test guidelines)
• This PR includes API docs for newly added methods/properties (api docs guidelines)
• This PR includes feature/README.MD updates for the feature docs
• This PR includes general feature table updates in the root README.MD
• This PR includes CHANGELOG.MD updates for newly added functionality
• This PR contains breaking changes
• This PR includes ng update migrations for the breaking changes (migrations guidelines)
• This PR includes behavioral changes and the feature specification has been updated with them

[RivaIvanova]

Setting the selection via the select method emits the selectionChanging event. It does not matter if it is a valid selection or not. The event should be emitted only on user interaction.

[georgianastasov]

Setting the selection via the select method emits the selectionChanging event. It does not matter if it is a valid selection or not. The event should be emitted only on user interaction.

@RivaIvanova @jackofdiamond5
Currently, the select() method emits the selectionChanging event even when the selection is set programmatically, regardless of whether the selection is valid.

Are you suggesting that the selectionChanging event should only be emitted on user interactions and not when the selection is changed programmatically via the select() method? If so, would you like me to modify the select() method to prevent it from emitting this event during programmatic changes?

Please let me know if I’ve understood your suggestion correctly or if there’s another approach you’d prefer me to consider.

[RivaIvanova]

You are correct and the select method along with the selectAllItems, setSelectedItem, deselect, deselectAllItems methods emit the selectionChanging event prior to this PR so you can disregard the previous comment. However, there are some other behaviors that I observed when the combo has the required attribute set:

<igx-combo
  #comboModel
  #myComboModel="ngModel"
  required
  [data]="items"
  [(ngModel)]="ngModelValue"
>
  <label igxLabel>Fruits</label>
</igx-combo>

combo selection {{ comboModel.selection.length }}
combo valid {{ myComboModel.valid }}

public items: any[] = ['Mango', 'Banana', 'Apple'];
public ngModelValue = ['invalid value'];

In this case, the combo selection is 0, and nothing is displayed in the read-only input field, which is correct according to the new changes. However, the controls valid property is true but should be false.

If the custom ngModelValue is set to an empty array, i.e., public ngModelValue = [];, the valid property is properly set to false.

The same is true if we have a combo inside a reactive form.

<form [formGroup]="user">
  <igx-combo
    [data]="genres"
    formControlName="genres"
  >
    <label igxLabel>Movies</label>
  </igx-combo>
</form>
reactive form valid {{ user.valid }}

public user: UntypedFormGroup;
public genres: any[] = ['Action', 'Comedy', 'Adventure'];

constructor(fb: UntypedFormBuilder) {
  this.user = fb.group({
    genres: [['test'], Validators.required],
  });
}

If we set genres to an empty array genres: [[], Validators.required], the reactive form's valid property is false.

Here is a sample that demonstrates the configuration.

Also, this enhancement aligns the Angular combo with the WC one, which is great, but this should be addressed for the simple combo as well. 🙂

[georgianastasov]

We have separated this behavior into a separate issue #14949 to ensure it is considered independently and does not block the new enhancement. The enhancement is already implemented and ready for review.

[jackofdiamond5]

This one directly modifies the value of the field bound to the model and the one below changes the value through an API call. So, we should change the tests' descriptions accordingly.

[jackofdiamond5]

At this point the select methods for both combos are very similar. So, it should be safe to extract the logic to a common method.

[jackofdiamond5]

Should we check if we also have the data loaded here, like we do with the other combo 🤔 In which case, it could be possible to have the writeValue method cease being abstract in combo.common as we could merge the logic of both methods into a common one.