From 6dbda3febcd4018cb4d0c2fbbd24ee7fd0ab4d4a Mon Sep 17 00:00:00 2001 From: Blackbaud-SteveBrush Date: Mon, 28 Oct 2024 14:06:03 -0400 Subject: [PATCH 1/2] fix(components/lookup): fix lookup component to emit only once per change --- .../modules/lookup/lookup.component.spec.ts | 51 +++++++++ .../lib/modules/lookup/lookup.component.ts | 105 ++++++++++-------- 2 files changed, 109 insertions(+), 47 deletions(-) diff --git a/libs/components/lookup/src/lib/modules/lookup/lookup.component.spec.ts b/libs/components/lookup/src/lib/modules/lookup/lookup.component.spec.ts index 56f7356c62..822ffbef32 100644 --- a/libs/components/lookup/src/lib/modules/lookup/lookup.component.spec.ts +++ b/libs/components/lookup/src/lib/modules/lookup/lookup.component.spec.ts @@ -965,6 +965,57 @@ describe('Lookup component', function () { component.setSingleSelect(); }); + it('should emit only once per value change', fakeAsync(() => { + const bestFriend = { name: 'Rachel' }; + component.friends = [bestFriend]; + + let counter = 0; + + const sub = component.form.valueChanges.subscribe(() => { + counter++; + }); + + // Expect zero on init. + fixture.detectChanges(); + expect(counter).toEqual(0); + counter = 0; + + // Programmatic change: + component.form.setValue({ + friends: [{ name: 'Xavier' }], + }); + fixture.detectChanges(); + expect(counter).toEqual(1); + counter = 0; + + // User interaction: + performSearch('s', fixture); + selectSearchResult(0, fixture); + expect(counter).toEqual(1); + counter = 0; + + // Programmatic, set to null: + component.form.setValue({ + friends: null, + }); + fixture.detectChanges(); + expect(counter).toEqual(1); + counter = 0; + + // Programmatic, but do not notify: + component.form.setValue( + { + friends: [{ name: 'Isaac' }], + }, + { emitEvent: false }, + ); + fixture.detectChanges(); + expect(counter).toEqual(0); + counter = 0; + + sub.unsubscribe(); + })); + it('should allow a preselected value', fakeAsync(() => { const bestFriend = { name: 'Rachel' }; expect(lookupComponent.value).toEqual([]); diff --git a/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts b/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts index f729660d92..c5c5d48966 100644 --- a/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts +++ b/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts @@ -198,10 +198,12 @@ export class SkyLookupComponent this.#updateForSelectMode(); if (multipleToSingle) { - if (this.value && this.value.length > 1) { + const value = this.#getValue(); + + if (value?.length > 1) { // The `setTimeout` is needed to avoid a `ExpressionChangedAfterItHasBeenCheckedError` error in template forms. setTimeout(() => { - this.writeValue([this.value[0]]); + this.writeValue([value[0]]); this.#changeDetector.detectChanges(); }); } @@ -234,11 +236,13 @@ export class SkyLookupComponent return this.#_tokens; } - public set tokens(value: SkyToken[] | undefined) { + public set tokens(tokens: SkyToken[] | undefined) { + const value = this.#getValue(); + // Collapse the tokens into a single token if the user has selected many options. - if (this.enableShowMore && this.value.length > 5) { + if (this.enableShowMore && value.length > 5) { this.#resourcesService - .getString('skyux_lookup_tokens_summary', this.value.length.toString()) + .getString('skyux_lookup_tokens_summary', value.length.toString()) .pipe(take(1)) .subscribe((label) => { this.#_tokens = [ @@ -249,24 +253,18 @@ export class SkyLookupComponent this.#changeDetector.markForCheck(); }); } else { - this.#_tokens = value; + this.#_tokens = tokens; this.#changeDetector.markForCheck(); } } + /** + * @internal + * @deprecated This is exposed for unit tests only; refactor unit tests to + * derive the control value a different way. + */ public get value(): any[] { - return this.#_value ? this.#_value : []; - } - - public set value(newValue: any[]) { - this.#_value = newValue; - - if (!this.#pickerModalOpen()) { - this.tokens = this.#parseTokens(newValue); - } - - this.onChange(this.#_value); - this.onTouched(); + return this.#getValue(); } public autocompleteController = new Subject(); @@ -319,6 +317,8 @@ export class SkyLookupComponent #idle = new Subject(); #markForTokenFocusOnKeyUp = false; #ngUnsubscribe = new Subject(); + #notifyChange: ((value: any[]) => void) | undefined; + #notifyTouched: (() => void) | undefined; #openNativePicker: SkyModalInstance | undefined; #openSelectionModal: SkySelectionModalInstance | undefined; #_required = false; @@ -342,6 +342,22 @@ export class SkyLookupComponent readonly #selectionModalSvc = inject(SkySelectionModalService); readonly #windowRef = inject(SkyAppWindowRef); + #getValue(): any[] { + return this.#_value ? this.#_value : []; + } + + #setValue(newValue: any[], options: { emitEvent: boolean }): void { + this.#_value = newValue; + + if (this.selectMode === 'multiple' && !this.#pickerModalOpen()) { + this.tokens = this.#parseTokens(newValue); + } + + if (options.emitEvent) { + this.#notifyChange?.(this.#_value); + } + } + constructor( @Self() @Optional() ngControl?: NgControl, @Optional() public inputBoxHostSvc?: SkyInputBoxHostService, @@ -411,16 +427,15 @@ export class SkyLookupComponent public onAutocompleteSelectionChange( change: SkyAutocompleteSelectionChange, ): void { - /* istanbul ignore else */ if (change.selectedItem) { this.#addToSelected(change.selectedItem); - } else if (this.selectMode === 'single') { - this.writeValue([]); + } else { + this.#setValue([], { emitEvent: true }); } } public onAutocompleteBlur(): void { - this.onTouched(); + this.#notifyTouched?.(); } public onTokensChange(change: SkyToken[]): void { @@ -433,14 +448,12 @@ export class SkyLookupComponent this.#focusInput(); } - // NOTE: We do this here instead of just using the `value` setter because we need to use the - // set of tokens returned here for the purposes of setting focus (see `onTokensKeyUp`). - this.#_value = change.map((token) => { + this.tokens = change; + const value = change.map((token) => { return token.value; }); - this.tokens = change; - this.onChange(this.#_value); - this.onTouched(); + + this.#setValue(value, { emitEvent: true }); } } @@ -480,22 +493,16 @@ export class SkyLookupComponent public writeValue(value: any[]): void { // Since we are dealing with arrays - clone the array being sent in to ensure we aren't modifying a consumers outer array - this.value = value ? value.slice() : []; + this.#setValue(value ? value.slice() : [], { emitEvent: false }); this.#updateForSelectMode(); } - // Angular automatically constructs these methods. - // eslint-disable-next-line @typescript-eslint/no-empty-function - public onChange = (value: any[]) => {}; - // eslint-disable-next-line @typescript-eslint/no-empty-function - public onTouched = () => {}; - - public registerOnChange(fn: (value: any) => void) { - this.onChange = fn; + public registerOnChange(fn: (value: any[]) => void) { + this.#notifyChange = fn; } public registerOnTouched(fn: () => void): void { - this.onTouched = fn; + this.#notifyTouched = fn; } // Allows Angular to disable the input. @@ -610,10 +617,10 @@ export class SkyLookupComponent this.showMoreConfig.customPicker.open({ items: this.data, initialSearch, - initialValue: this.value, + initialValue: this.#getValue(), }); } else { - const initialValue = this.value; + const initialValue = this.#getValue(); if (this.#hasSearchAsync()) { this.#openSelectionModal = @@ -676,7 +683,7 @@ export class SkyLookupComponent #createSelectionModalInstance( initialSearch: string, ): SkySelectionModalInstance { - const initialValue = this.value; + const initialValue = this.#getValue(); const modalConfig = this.showMoreConfig?.nativePickerConfig || {}; if (!modalConfig.itemTemplate) { @@ -719,7 +726,7 @@ export class SkyLookupComponent } #createNativePickerInstance(initialSearch: string): SkyModalInstance { - const initialValue = this.value; + const initialValue = this.#getValue(); const modalConfig = this.showMoreConfig?.nativePickerConfig || {}; if (!modalConfig.itemTemplate) { @@ -776,19 +783,21 @@ export class SkyLookupComponent } #addToSelected(item: any): void { + const value = this.#getValue(); + let selectedItems: any[]; if (this.selectMode === 'single') { selectedItems = [item]; } else { - selectedItems = this.value; + selectedItems = value; const idProperty = this.idProperty || ''; // If items have a unique identifier, don't allow the same item to be added twice. if ( !this.idProperty || - !this.value.some( + !value.some( (existingItem) => existingItem[idProperty] === item[idProperty], ) ) { @@ -798,7 +807,7 @@ export class SkyLookupComponent this.clearSearchText(); } - this.writeValue(selectedItems); + this.#setValue(selectedItems, { emitEvent: true }); } #addEventListeners(): void { @@ -894,9 +903,10 @@ export class SkyLookupComponent } if (addItemToValue) { + const oldValue = this.#getValue(); const newValue = this.selectMode === 'multiple' - ? this.value.concat(args.item) + ? oldValue.concat(args.item) : [args.item]; this.writeValue(newValue); } @@ -922,7 +932,8 @@ export class SkyLookupComponent #updateForSelectMode(): void { if (this.autocompleteInputDirective) { if (this.selectMode === 'single') { - this.autocompleteInputDirective.value = this.value && this.value[0]; + const value = this.#getValue(); + this.autocompleteInputDirective.value = value && value[0]; } else { this.clearSearchText(); } From b800a6c757d1192975c11e05314065e2581cbf21 Mon Sep 17 00:00:00 2001 From: Blackbaud-SteveBrush Date: Mon, 28 Oct 2024 15:04:54 -0400 Subject: [PATCH 2/2] PR feedback --- .../lookup/src/lib/modules/lookup/lookup.component.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts b/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts index c5c5d48966..adb4ab1feb 100644 --- a/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts +++ b/libs/components/lookup/src/lib/modules/lookup/lookup.component.ts @@ -356,6 +356,8 @@ export class SkyLookupComponent if (options.emitEvent) { this.#notifyChange?.(this.#_value); } + + this.#updateForSelectMode(); } constructor( @@ -494,7 +496,6 @@ export class SkyLookupComponent public writeValue(value: any[]): void { // Since we are dealing with arrays - clone the array being sent in to ensure we aren't modifying a consumers outer array this.#setValue(value ? value.slice() : [], { emitEvent: false }); - this.#updateForSelectMode(); } public registerOnChange(fn: (value: any[]) => void) {