From 5f6409d9860771f7471bffb3811ed0947c5a3415 Mon Sep 17 00:00:00 2001 From: Thomas Heller Date: Thu, 16 Jan 2020 06:07:41 +0000 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 | 5 ++ .../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, 154 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 eb69b8b8f8..c687baee59 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 @@ -26,6 +26,7 @@ import { focusFilterFieldInput, getFilterfieldTags, tagOverlay, + setupSecondTestScenario, } from './filter-field.po'; import { Selector } from 'testcafe'; import { waitForAngular } from '../../utils'; @@ -256,3 +257,30 @@ test('should show the overlay on a tag because the tag value is ellipsed', async .expect(tagOverlay.exists) .ok(); }); + +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. + .wait(500); + + // Manually set an option, because this seems to break the + // clear all change detection. + await clickOption(1); + await clickOption(1) + .expect(filterTags.count) + .eql(2) + // Click somewhere outside so the clear-all button appears + .click(Selector('.outside')) + // Wait for the filterfield to catch up. + .wait(500) + .expect(clearAll.exists) + .ok() + // Click the clear all-button, the created filter should be removed + .click(clearAll) + // Wait for the filterfield to catch up. + .wait(500) + .expect(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 187666faa8..7ce1a0386c 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 @@ -25,11 +25,16 @@ export const tagOverlay = Selector('.dt-overlay-container'); export const input = Selector('input'); +export const switchToFirstDatasource = Selector('#switchToFirstDatasource'); +export const switchToSecondDatasource = Selector('#switchToSecondDatasource'); +export const setupSecondTestScenario = Selector('#setupSecondTestScenario'); + export function clickOption( nth: number, testController?: TestController, ): TestControllerPromise { const controller = testController || t; + return controller .click(filterField) .wait(150) 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 3774b3b190..c9d4020fe5 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', @@ -70,10 +78,84 @@ const TEST_DATA: DtFilterFieldDefaultDataSourceType = { ], }; +const TEST_DATA_2 = { + autocomplete: [ + { + name: 'AUT', + distinct: true, + autocomplete: [{ name: 'Linz' }, { name: 'Vienna' }, { name: 'Graz' }], + }, + { + name: 'USA', + autocomplete: [ + { name: 'San Francisco' }, + { name: 'Los Angeles' }, + { name: '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 0a1a4b6c44..c33cd4b5da 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 2aa8f61b4e..915bb0291d 100644 --- a/components/filter-field/src/filter-field.scss +++ b/components/filter-field/src/filter-field.scss @@ -112,6 +112,11 @@ button.dt-filter-field-clear-all-button { line-height: 22px; } +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 694e27de18..cee8ebcbfc 100644 --- a/components/filter-field/src/filter-field.spec.ts +++ b/components/filter-field/src/filter-field.spec.ts @@ -1654,7 +1654,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], @@ -1665,7 +1666,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 @@ -1684,7 +1686,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 @@ -1707,7 +1709,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', () => { @@ -1721,7 +1723,7 @@ describe('DtFilterField', () => { fixture.componentInstance.clearAllLabel = ''; fixture.detectChanges(); - expect(getClearAll(fixture)).toBeNull(); + expect(isClearAllVisible(fixture)).toBe(false); }); it('should reset the entire filter field', () => { @@ -2142,13 +2144,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'), @@ -2156,6 +2157,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: `