From 0724f5ac0e8a4709d3df5729f2750128c462bcc8 Mon Sep 17 00:00:00 2001 From: Thomas Heller Date: Thu, 16 Jan 2020 07:07:41 +0100 Subject: [PATCH] fix(filter-field): Fixes an issue with inconsistent clearAll functionality. The clearAll button rendering depended on the filterfield not being focussed. When the clearAll button was rendered and was clicked, the cdkFocusMonitor kicked in, set the focus and removed the clearAll button again. As a result the clearAll button lost its focus, the document focus was set back to the body and the clearAll was rendered again. The click event on the clearAll was never fired. We no longer remove the clearAll from the DOM, but hide it via css classes. This makes it not lose focus, and results in a more consistent behavior. Fixes #435 --- .../filter-field/filter-field.e2e.ts | 28 ++++++ .../components/filter-field/filter-field.html | 11 +++ .../filter-field/filter-field.po.ts | 4 + .../components/filter-field/filter-field.ts | 90 ++++++++++++++++++- components/filter-field/src/filter-field.html | 3 +- components/filter-field/src/filter-field.scss | 5 ++ .../filter-field/src/filter-field.spec.ts | 24 +++-- 7 files changed, 153 insertions(+), 12 deletions(-) diff --git a/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts b/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts index 860b19f809..6a4839e993 100644 --- a/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts +++ b/apps/components-e2e/src/components/filter-field/filter-field.e2e.ts @@ -23,6 +23,7 @@ import { input, clearAll, filterTags, + setupSecondTestScenario, } from './filter-field.po'; import { Selector } from 'testcafe'; @@ -76,3 +77,30 @@ test('should remove all filters when clicking the clear-all button', async (test await testController.wait(300); await testController.expect(await filterTags.exists).notOk(); }); + +test('should remove all removable filters when clicking the clear-all button', async (testController: TestController) => { + // Setup the second datasource. + await testController.click(setupSecondTestScenario); + + // Wait for the filterfield to catch up. + await testController.wait(500); + + // Manually set an option, because this seems to break the + // clear all change detection. + await clickOption(1); + await clickOption(1); + + await testController.expect(await filterTags.count).eql(2); + + // // Click somewhere outside so the clear-all button appears + await testController.click(Selector('.outside')); + // Wait for the filterfield to catch up. + await testController.wait(500); + await testController.expect(await clearAll.exists).ok(); + + // Click the clear all-button, the created filter should be removed + await testController.click(clearAll); + // Wait for the filterfield to catch up. + await testController.wait(500); + await testController.expect(await filterTags.count).eql(1); +}); diff --git a/apps/components-e2e/src/components/filter-field/filter-field.html b/apps/components-e2e/src/components/filter-field/filter-field.html index efc1e12f34..8ba219efa2 100644 --- a/apps/components-e2e/src/components/filter-field/filter-field.html +++ b/apps/components-e2e/src/components/filter-field/filter-field.html @@ -6,3 +6,14 @@ label="Filter by" clearAllLabel="Clear all" > + + + + + diff --git a/apps/components-e2e/src/components/filter-field/filter-field.po.ts b/apps/components-e2e/src/components/filter-field/filter-field.po.ts index 31657239b0..fe5fc1b682 100644 --- a/apps/components-e2e/src/components/filter-field/filter-field.po.ts +++ b/apps/components-e2e/src/components/filter-field/filter-field.po.ts @@ -24,6 +24,10 @@ export const filterTags = Selector('dt-filter-field-tag'); export const input = Selector('input'); +export const switchToFirstDatasource = Selector('#switchToFirstDatasource'); +export const switchToSecondDatasource = Selector('#switchToSecondDatasource'); +export const setupSecondTestScenario = Selector('#setupSecondTestScenario'); + export async function clickOption( nth: number, testController?: TestController, diff --git a/apps/components-e2e/src/components/filter-field/filter-field.ts b/apps/components-e2e/src/components/filter-field/filter-field.ts index dfb422dcd7..e6fa2f1254 100644 --- a/apps/components-e2e/src/components/filter-field/filter-field.ts +++ b/apps/components-e2e/src/components/filter-field/filter-field.ts @@ -17,15 +17,23 @@ // tslint:disable no-lifecycle-call no-use-before-declare no-magic-numbers deprecation // tslint:disable no-any max-file-line-count no-unbound-method use-component-selector -import { Component } from '@angular/core'; +import { + Component, + ViewChild, + ChangeDetectorRef, + OnDestroy, +} from '@angular/core'; import { Validators } from '@angular/forms'; import { DtFilterFieldDefaultDataSource, DtFilterFieldDefaultDataSourceType, + DtFilterField, } from '@dynatrace/barista-components/filter-field'; +import { takeUntil } from 'rxjs/operators'; +import { Subject } from 'rxjs'; -const TEST_DATA: DtFilterFieldDefaultDataSourceType = { +const TEST_DATA = { autocomplete: [ { name: 'custom normal', @@ -57,10 +65,84 @@ const TEST_DATA: DtFilterFieldDefaultDataSourceType = { ], }; +const TEST_DATA_2 = { + autocomplete: [ + { + name: 'AUT', + distinct: true, + autocomplete: ['Linz', 'Vienna', 'Graz'], + }, + { + name: 'USA', + autocomplete: [ + 'San Francisco', + 'Los Angeles', + 'New York', + { name: 'Custom', suggestions: [] }, + ], + }, + { + name: 'Requests per minute', + range: { + operators: { + range: true, + equal: true, + greaterThanEqual: true, + lessThanEqual: true, + }, + unit: 's', + }, + }, + ], +}; + +const DATA = [TEST_DATA, TEST_DATA_2]; + @Component({ selector: 'dt-e2e-filter-field', templateUrl: './filter-field.html', }) -export class DtE2EFilterField { - _dataSource = new DtFilterFieldDefaultDataSource(TEST_DATA); +export class DtE2EFilterField implements OnDestroy { + destroy$ = new Subject(); + + _dataSource = new DtFilterFieldDefaultDataSource< + DtFilterFieldDefaultDataSourceType + >(DATA[0]); + + @ViewChild(DtFilterField, { static: true }) _filterfield: DtFilterField< + DtFilterFieldDefaultDataSourceType + >; + + switchToDatasource(targetIndex: number): void { + this._dataSource = new DtFilterFieldDefaultDataSource< + DtFilterFieldDefaultDataSourceType + >(DATA[targetIndex]); + } + + constructor(private _changeDetectorRef: ChangeDetectorRef) {} + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + } + + setupSecondTestScenario(): void { + this._dataSource = new DtFilterFieldDefaultDataSource(DATA[1]); + const linzFilter = [ + DATA[1].autocomplete[0], + DATA[1].autocomplete[0].autocomplete![0], + ]; + + this._filterfield.filters = [linzFilter]; + this._filterfield.currentTags + .pipe(takeUntil(this.destroy$)) + .subscribe(() => { + const linzTag = this._filterfield.getTagForFilter(linzFilter); + if (linzTag) { + linzTag.editable = false; + linzTag.deletable = false; + this._changeDetectorRef.markForCheck(); + } + }); + } } diff --git a/components/filter-field/src/filter-field.html b/components/filter-field/src/filter-field.html index a61db15645..8674bc7d04 100644 --- a/components/filter-field/src/filter-field.html +++ b/components/filter-field/src/filter-field.html @@ -20,7 +20,8 @@ variant="secondary" class="dt-filter-field-clear-all-button" (click)="_clearAll()" - *ngIf="_showClearAll" + *ngIf="clearAllLabel" + [class.dt-filter-field-clear-all-button-hidden]="!_showClearAll" >{{clearAllLabel}}
diff --git a/components/filter-field/src/filter-field.scss b/components/filter-field/src/filter-field.scss index 5ff0b168ef..83f92ae667 100644 --- a/components/filter-field/src/filter-field.scss +++ b/components/filter-field/src/filter-field.scss @@ -110,6 +110,11 @@ button.dt-filter-field-clear-all-button { margin-left: 8px; } +button.dt-filter-field-clear-all-button-hidden { + visibility: hidden; + position: absolute; +} + // We need to double the class to get a better specificity // than the original autocomplete panel selector ::ng-deep .dt-autocomplete-panel.dt-filter-field-panel.dt-filter-field-panel { diff --git a/components/filter-field/src/filter-field.spec.ts b/components/filter-field/src/filter-field.spec.ts index 9503a859de..23f9a1abf0 100644 --- a/components/filter-field/src/filter-field.spec.ts +++ b/components/filter-field/src/filter-field.spec.ts @@ -1611,7 +1611,8 @@ describe('DtFilterField', () => { }); it('should not display the clear all button if no filters are set', () => { - expect(getClearAll(fixture)).toBeNull(); + // Check if the clear all is initially not visible. + expect(isClearAllVisible(fixture)).toBe(false); const autocompleteFilter = [ TEST_DATA_EDITMODE.autocomplete[0], @@ -1622,7 +1623,8 @@ describe('DtFilterField', () => { filterField.filters = [autocompleteFilter]; fixture.detectChanges(); - expect(getClearAll(fixture)).not.toBeNull(); + // After setting the filters, there should be a clear all button visible. + expect(isClearAllVisible(fixture)).toBe(true); }); // the user is in the edit mode of a filter @@ -1641,7 +1643,7 @@ describe('DtFilterField', () => { zone.simulateZoneExit(); fixture.detectChanges(); - expect(getClearAll(fixture)).toBeNull(); + expect(isClearAllVisible(fixture)).toBe(false); }); // the user is in the edit mode of a filter @@ -1664,7 +1666,7 @@ describe('DtFilterField', () => { const autOption = options[0]; autOption.click(); - expect(getClearAll(fixture)).toBeNull(); + expect(isClearAllVisible(fixture)).toBe(false); }); it('should not display the clear all button if no label is provided', () => { @@ -1678,7 +1680,7 @@ describe('DtFilterField', () => { fixture.componentInstance.clearAllLabel = ''; fixture.detectChanges(); - expect(getClearAll(fixture)).toBeNull(); + expect(isClearAllVisible(fixture)).toBe(false); }); it('should reset the entire filter field', () => { @@ -2094,13 +2096,12 @@ function getTagButtons( return { label, deleteButton }; } -// tslint:disable-next-line:no-any function getInput(fixture: ComponentFixture): HTMLInputElement { return fixture.debugElement.query(By.css('.dt-filter-field-input')) .nativeElement; } -// tslint:disable-next-line:no-any +/** Get the clearAll button. */ function getClearAll(fixture: ComponentFixture): HTMLButtonElement | null { const dbgEl = fixture.debugElement.query( By.css('.dt-filter-field-clear-all-button'), @@ -2108,6 +2109,15 @@ function getClearAll(fixture: ComponentFixture): HTMLButtonElement | null { return dbgEl ? dbgEl.nativeElement : null; } +/** Get the clearAll button and evaluate if it is visible or not. */ +function isClearAllVisible(fixture: ComponentFixture): boolean { + const clearAll = getClearAll(fixture); + return ( + clearAll !== null && + !clearAll.classList.contains('dt-filter-field-clear-all-button-hidden') + ); +} + @Component({ selector: 'test-app', template: `