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

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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

georgianastasov
Copy link
Contributor

@georgianastasov georgianastasov commented Sep 16, 2024

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 RivaIvanova added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification 💥 status: in-test PRs currently being tested labels Oct 8, 2024
@RivaIvanova
Copy link
Member

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 RivaIvanova added the 🛠️ status: in-development Issues and PRs with active development on them label Oct 8, 2024
@georgianastasov
Copy link
Contributor Author

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 RivaIvanova added 💥 status: in-test PRs currently being tested and removed 🛠️ status: in-development Issues and PRs with active development on them labels Oct 9, 2024
@RivaIvanova
Copy link
Member

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.

igxCombo - required

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. 🙂

@RivaIvanova RivaIvanova added 🛠️ status: in-development Issues and PRs with active development on them and removed 💥 status: in-test PRs currently being tested labels Oct 9, 2024
@georgianastasov georgianastasov changed the title feat(combo): filter out non-existent items from selection - master feat(combo, simple-combo): filter out non-existent items from selection - master Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
combo 🛠️ status: in-development Issues and PRs with active development on them version: 18.2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatch in combo/simple-combo: Non-matching values should not appear in the selection
3 participants