From e84886aab683085121643f20b60dcc308a297d80 Mon Sep 17 00:00:00 2001 From: Ivan Donchev Date: Wed, 1 Feb 2023 15:26:12 +0200 Subject: [PATCH] fix(combobox): do not prefilter when first opened closes: #386 BREAKING CHANGE: the combobox list is no longer prefiltered on first dialog open, but shows all options. Signed-off-by: Ivan Donchev --- .../angular/src/forms/combobox/combobox.ts | 10 +++++++- .../combobox/option-items.directive.spec.ts | 24 +++++++++---------- .../forms/combobox/option-items.directive.ts | 4 +++- .../providers/option-selection.service.ts | 3 +++ 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/projects/angular/src/forms/combobox/combobox.ts b/projects/angular/src/forms/combobox/combobox.ts index 53fa8e09b8..7a92719360 100644 --- a/projects/angular/src/forms/combobox/combobox.ts +++ b/projects/angular/src/forms/combobox/combobox.ts @@ -137,10 +137,16 @@ export class ClrCombobox set searchText(text: string) { // if input text has changed since last time, fire a change event so application can react to it if (text !== this._searchText) { + if (this.toggleService.open) { + this.optionSelectionService.showAllOptions = false; + } this._searchText = text; this.clrInputChange.emit(this.searchText); - this.optionSelectionService.currentInput = this.searchText; } + // We need to trigger this even if unchanged, so the option-items directive will update its list + // based on the "showAllOptions" variable which may have changed in the openChange subscription below. + // The option-items directive does not listen to openChange, but it listens to currentInput changes. + this.optionSelectionService.currentInput = this.searchText; } get searchText(): string { @@ -273,6 +279,8 @@ export class ClrCombobox this.toggleService.openChange.subscribe(open => { if (open) { this.focusFirstActive(); + } else { + this.optionSelectionService.showAllOptions = true; } if (this.multiSelect) { this.searchText = ''; diff --git a/projects/angular/src/forms/combobox/option-items.directive.spec.ts b/projects/angular/src/forms/combobox/option-items.directive.spec.ts index e6b77fd7f1..225ddf17e2 100644 --- a/projects/angular/src/forms/combobox/option-items.directive.spec.ts +++ b/projects/angular/src/forms/combobox/option-items.directive.spec.ts @@ -84,18 +84,7 @@ export default function (): void { this.fixture.detectChanges(); const updatedContent = this.fixture.elementRef.nativeElement.textContent; - /* This took me quite some time to research, so it needs a detailed explanation. - The data not updating immediately does not mean that the new value will not be added to the combobox. - It only means that it won't be added to the currently open popover. Which we do not want anyway, as: - - it will cause flickering and focus loss/mess issues; - - for performance reasons we're only updating the iterator on input change (replace, pushing does not - count) and on filter change; - - the user still has the workaround to replace the input reference instead of pushing. - Based on the above, I prefer to avoid complicating the iterator, unless we have a real scenario for it. - */ - // Deprecated check: - // expect(updatedContent.trim()).toEqual('01236'); - expect(updatedContent.trim()).toEqual('0123'); + expect(updatedContent.trim()).toEqual('01236'); }); it('handles a null input for the array of items', function () { @@ -110,9 +99,19 @@ export default function (): void { expect(this.clarityDirective._rawItems).toEqual([]); }); + it('will not filter on first open', function () { + expect(this.clarityDirective.iterableProxy._ngForOf).toEqual([0, 1, 2, 3]); + const optionService: OptionSelectionService = TestBed.get(OptionSelectionService); + this.testComponent.numbers.push(12); + optionService.currentInput = '1'; + this.fixture.detectChanges(); + expect(this.clarityDirective.iterableProxy._ngForOf).toEqual([0, 1, 2, 3, 12]); + }); + it('can filter out items based on the option service currentInput field', function () { expect(this.clarityDirective.iterableProxy._ngForOf).toEqual([0, 1, 2, 3]); const optionService: OptionSelectionService = TestBed.get(OptionSelectionService); + optionService.showAllOptions = false; this.testComponent.numbers.push(12); optionService.currentInput = '1'; this.fixture.detectChanges(); @@ -121,6 +120,7 @@ export default function (): void { it('has case insensive filter', function () { const optionService: OptionSelectionService = TestBed.get(OptionSelectionService); + optionService.showAllOptions = false; this.testComponent.numbers.push('Room', 'Broom'); optionService.currentInput = 'ro'; this.fixture.detectChanges(); diff --git a/projects/angular/src/forms/combobox/option-items.directive.ts b/projects/angular/src/forms/combobox/option-items.directive.ts index 30e4c34bcf..342bf89f88 100644 --- a/projects/angular/src/forms/combobox/option-items.directive.ts +++ b/projects/angular/src/forms/combobox/option-items.directive.ts @@ -70,7 +70,9 @@ export class ClrOptionItems implements DoCheck, OnDestroy { if (!this._rawItems || this.filter === undefined || this.filter === null) { return; } - if (this._filterField) { + if (this.optionService.showAllOptions) { + this.filteredItems = this._rawItems; + } else if (this._filterField) { this.filteredItems = this._rawItems.filter(item => { const objValue = (item as any)[this._filterField]; return objValue ? objValue.toString().toLowerCase().indexOf(this.filter.toLowerCase().toString()) > -1 : false; diff --git a/projects/angular/src/forms/combobox/providers/option-selection.service.ts b/projects/angular/src/forms/combobox/providers/option-selection.service.ts index 0527b43a19..19ed8dc6ff 100644 --- a/projects/angular/src/forms/combobox/providers/option-selection.service.ts +++ b/projects/angular/src/forms/combobox/providers/option-selection.service.ts @@ -15,6 +15,9 @@ export class OptionSelectionService { selectionModel: ComboboxModel; loading = false; displayField: string; + // Display all options on first open, even if filter text exists. + // https://github.com/vmware-clarity/ng-clarity/issues/386 + showAllOptions = true; private _currentInput = ''; get currentInput(): string { return this._currentInput;