Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
fix(filter-field): Fixes an issue with inconsistent clearAll function…
Browse files Browse the repository at this point in the history
…ality.

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
  • Loading branch information
tomheller authored and ffriedl89 committed Feb 4, 2020
1 parent 5e7c4a6 commit 5f6409d
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
focusFilterFieldInput,
getFilterfieldTags,
tagOverlay,
setupSecondTestScenario,
} from './filter-field.po';
import { Selector } from 'testcafe';
import { waitForAngular } from '../../utils';
Expand Down Expand Up @@ -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);
});
11 changes: 11 additions & 0 deletions apps/components-e2e/src/components/filter-field/filter-field.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,14 @@
label="Filter by"
clearAllLabel="Clear all"
></dt-filter-field>

<button id="switchToFirstDatasource" (click)="switchToDatasource(0)">
Switch to first datasource
</button>
<button id="switchToSecondDatasource" (click)="switchToDatasource(1)">
Switch to second datasource
</button>

<button id="setupSecondTestScenario" (click)="setupSecondTestScenario()">
setupSecondTestScenario
</button>
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
90 changes: 86 additions & 4 deletions apps/components-e2e/src/components/filter-field/filter-field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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<any>(TEST_DATA);
export class DtE2EFilterField implements OnDestroy {
destroy$ = new Subject<void>();

_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();
}
});
}
}
3 changes: 2 additions & 1 deletion components/filter-field/src/filter-field.html
Original file line number Diff line number Diff line change
Expand Up @@ -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}}</button>

<div class="dt-filter-field-infix">
Expand Down
5 changes: 5 additions & 0 deletions components/filter-field/src/filter-field.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
24 changes: 17 additions & 7 deletions components/filter-field/src/filter-field.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -2142,20 +2144,28 @@ function getTagButtons(
return { label, deleteButton };
}

// tslint:disable-next-line:no-any
function getInput(fixture: ComponentFixture<any>): 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<any>): HTMLButtonElement | null {
const dbgEl = fixture.debugElement.query(
By.css('.dt-filter-field-clear-all-button'),
);
return dbgEl ? dbgEl.nativeElement : null;
}

/** Get the clearAll button and evaluate if it is visible or not. */
function isClearAllVisible(fixture: ComponentFixture<any>): boolean {
const clearAll = getClearAll(fixture);
return (
clearAll !== null &&
!clearAll.classList.contains('dt-filter-field-clear-all-button-hidden')
);
}

@Component({
selector: 'test-app',
template: `
Expand Down

0 comments on commit 5f6409d

Please sign in to comment.