Skip to content

Fix multi select view update #1228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 2, 2021
Merged

Fix multi select view update #1228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d26ab0c
fix: multiselect updating trigger label
jake-bassett Nov 1, 2021
3872f99
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into fix-m…
jake-bassett Nov 1, 2021
6c7e235
style: prettier
jake-bassett Nov 1, 2021
68a0bac
feat: remove subscription
jake-bassett Nov 2, 2021
dcae038
test: fix test
jake-bassett Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 57 additions & 9 deletions projects/components/src/multi-select/multi-select.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { CommonModule } from '@angular/common';
import { fakeAsync, flush } from '@angular/core/testing';
import { IconLibraryTestingModule, IconType } from '@hypertrace/assets-library';
import { NavigationService } from '@hypertrace/common';
import { PopoverComponent } from '@hypertrace/components';
import { runFakeRxjs } from '@hypertrace/test-utils';
import { createHostFactory, mockProvider, SpectatorHost } from '@ngneat/spectator/jest';
import { MockComponent } from 'ng-mocks';
import { NEVER } from 'rxjs';
Expand All @@ -12,7 +14,6 @@ import { CheckboxComponent } from '../checkbox/checkbox.component';
import { DividerComponent } from '../divider/divider.component';
import { LabelComponent } from '../label/label.component';
import { LoadAsyncModule } from '../load-async/load-async.module';
import { PopoverComponent } from '../popover/popover.component';
import { PopoverModule } from '../popover/popover.module';
import { SearchBoxComponent } from '../search-box/search-box.component';
import { SelectOptionComponent } from '../select/select-option.component';
Expand Down Expand Up @@ -69,15 +70,20 @@ describe('Multi Select Component', () => {

spectator.tick();

expect(spectator.component.triggerLabel).toEqual(selectionOptions[1].label);
expect(spectator.query('.trigger-content')).toExist();
expect(spectator.query('.trigger-label-container')).toExist();
expect(spectator.query('.trigger-label')).toExist();
expect(spectator.query('.trigger-icon')).toExist();
expect(spectator.query('.trigger-more-items')).not.toExist();

// Selected element is 1 as set in hostProps
expect(spectator.component.selectedItemsCount).toBe(1);
runFakeRxjs(({ expectObservable }) => {
expectObservable(spectator.component.triggerValues$).toBe('x', {
x: {
label: selectionOptions[1].label,
selectedItemsCount: 1 // Selected element is 1 as set in hostProps
}
});
});

const popoverComponent = spectator.query(PopoverComponent);
expect(popoverComponent?.closeOnClick).toEqual(false);
Expand All @@ -101,11 +107,19 @@ describe('Multi Select Component', () => {
spectator.tick();
const selectedCheckboxElements = spectator.queryAll('ht-checkbox', { root: true });
expect(spectator.query('.trigger-more-items')).toExist();
expect(spectator.component.selectedItemsCount).toBe(2);
expect(
selectedCheckboxElements.filter(checkboxElement => checkboxElement.getAttribute('ng-reflect-checked') === 'true')
.length
).toBe(2);

runFakeRxjs(({ expectObservable }) => {
expectObservable(spectator.component.triggerValues$).toBe('x', {
x: {
label: selectionOptions[1].label,
selectedItemsCount: 2
}
});
});
}));

test('should display provided options with icons when clicked', fakeAsync(() => {
Expand Down Expand Up @@ -136,12 +150,20 @@ describe('Multi Select Component', () => {

const selectedCheckboxElements = spectator.queryAll('ht-checkbox', { root: true });
expect(spectator.query('.trigger-more-items')).toExist();
expect(spectator.component.selectedItemsCount).toBe(2);
expect(
selectedCheckboxElements.filter(checkboxElement => checkboxElement.getAttribute('ng-reflect-checked') === 'true')
.length
).toBe(2);

runFakeRxjs(({ expectObservable }) => {
expectObservable(spectator.component.triggerValues$).toBe('x', {
x: {
label: 'second',
selectedItemsCount: 2
}
});
});

optionElements.forEach((element, index) => {
expect(element).toHaveText(selectionOptions[index].label);
expect(element.querySelector('ht-icon')).toExist();
Expand Down Expand Up @@ -204,8 +226,17 @@ describe('Multi Select Component', () => {
expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange).toHaveBeenCalledWith([selectionOptions[1].value, selectionOptions[2].value]);
expect(spectator.query('.trigger-more-items')).toExist();
expect(spectator.component.selectedItemsCount).toBe(2);
expect(spectator.query(LabelComponent)?.label).toEqual('second');

runFakeRxjs(({ expectObservable }) => {
expectObservable(spectator.component.triggerValues$).toBe('x', {
x: {
label: 'second',
selectedItemsCount: 2
}
});
});

flush();
}));

Expand Down Expand Up @@ -332,7 +363,16 @@ describe('Multi Select Component', () => {
expect(onChange).toHaveBeenCalledWith([selectionOptions[1].value, selectionOptions[2].value]);
expect(spectator.query(LabelComponent)?.label).toEqual('Placeholder');
expect(spectator.query('.trigger-more-items')).not.toExist();
expect(spectator.component.selectedItemsCount).toBe(0);

runFakeRxjs(({ expectObservable }) => {
expectObservable(spectator.component.triggerValues$).toBe('(x|)', {
x: {
label: 'Placeholder',
selectedItemsCount: 0
}
});
});

flush();
}));

Expand All @@ -354,7 +394,15 @@ describe('Multi Select Component', () => {
);
spectator.tick();

expect(spectator.component.triggerLabel).toEqual(selectionOptions[1].label);
runFakeRxjs(({ expectObservable }) => {
expectObservable(spectator.component.triggerValues$).toBe('x', {
x: {
label: selectionOptions[1].label,
selectedItemsCount: 1
}
});
});

expect(spectator.query('.trigger-content')).toExist();
expect(spectator.query('.trigger-label-container')).toExist();
expect(spectator.query('.trigger-label')).toExist();
Expand Down
52 changes: 32 additions & 20 deletions projects/components/src/multi-select/multi-select.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import {
QueryList
} from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { queryListAndChanges$ } from '@hypertrace/common';
import { BehaviorSubject, combineLatest, EMPTY, Observable, Subject } from 'rxjs';
import { queryListAndChanges$, SubscriptionLifecycle } from '@hypertrace/common';
import { BehaviorSubject, combineLatest, EMPTY, Observable, of, Subject } from 'rxjs';
import { map } from 'rxjs/operators';
import { ButtonRole, ButtonStyle } from '../button/button';
import { IconSize } from '../icon/icon-size';
Expand All @@ -24,6 +24,7 @@ import { MultiSelectJustify } from './multi-select-justify';
selector: 'ht-multi-select',
styleUrls: ['./multi-select.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
providers: [SubscriptionLifecycle],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next time you're in here, this can be cleaned out

template: `
<div
class="multi-select"
Expand All @@ -47,14 +48,16 @@ import { MultiSelectJustify } from './multi-select-justify';
[ngClass]="[this.triggerLabelDisplayMode, this.popoverOpen ? 'open' : '']"
#triggerContainer
>
<ht-icon *ngIf="this.icon" [icon]="this.icon" [size]="this.iconSize"> </ht-icon>
<div *ngIf="!this.isIconOnlyMode()" class="trigger-label-container">
<ht-label class="trigger-label" [label]="this.triggerLabel"></ht-label>
<span *ngIf="this.selectedItemsCount > 1" class="trigger-more-items"
>+{{ this.selectedItemsCount - 1 }}</span
>
<ht-icon class="trigger-icon" icon="${IconType.ChevronDown}" size="${IconSize.Small}"></ht-icon>
</div>
<ht-icon *ngIf="this.icon" [icon]="this.icon" [size]="this.iconSize"></ht-icon>
<ng-container *htLoadAsync="this.triggerValues$ as triggerValues">
<div *ngIf="!this.isIconOnlyMode()" class="trigger-label-container">
<ht-label class="trigger-label" [label]="triggerValues.label"></ht-label>
<span *ngIf="triggerValues.selectedItemsCount > 1" class="trigger-more-items"
>+{{ triggerValues.selectedItemsCount - 1 }}</span
>
<ht-icon class="trigger-icon" icon="${IconType.ChevronDown}" size="${IconSize.Small}"></ht-icon>
</div>
</ng-container>
</div>
</ht-popover-trigger>
<ht-popover-content>
Expand Down Expand Up @@ -163,8 +166,7 @@ export class MultiSelectComponent<V> implements AfterContentInit, OnChanges {
private readonly searchSubject: Subject<string> = new BehaviorSubject('');

public popoverOpen: boolean = false;
public triggerLabel?: string;
public selectedItemsCount: number = 0;
public triggerValues$: Observable<TriggerValues> = new Observable();

public ngAfterContentInit(): void {
this.allOptions$ = this.allOptionsList !== undefined ? queryListAndChanges$(this.allOptionsList) : EMPTY;
Expand Down Expand Up @@ -236,22 +238,32 @@ export class MultiSelectComponent<V> implements AfterContentInit, OnChanges {

private setTriggerLabel(): void {
if (this.triggerLabelDisplayMode === TriggerLabelDisplayMode.Placeholder) {
this.triggerLabel = this.placeholder;
this.triggerValues$ = of({
label: this.placeholder,
selectedItemsCount: 0
});

return;
}

const selectedItems: SelectOptionComponent<V>[] | undefined = this.allOptionsList?.filter(item =>
this.isSelectedItem(item)
);
this.triggerValues$ = this.allOptions$?.pipe(
map(options => {
const selectedItems: SelectOptionComponent<V>[] = options.filter(item => this.isSelectedItem(item));

this.selectedItemsCount = selectedItems?.length ?? 0;

// Trigger label is placeholder in case there is element selected on multiselect
this.triggerLabel = this.selectedItemsCount === 0 ? this.placeholder : (selectedItems || [])[0]?.label;
return {
label: selectedItems.length === 0 ? this.placeholder : selectedItems[0]?.label,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is multi-select, IMO, this trigger should try to show all selected labels.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like <label1>,<label-2> and 5 more

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that it is already happening :D

selectedItemsCount: selectedItems.length
};
})
);
}
}

interface TriggerValues {
label: string | undefined;
selectedItemsCount: number;
}

export const enum TriggerLabelDisplayMode {
// These may be used as css classes
Placeholder = 'placeholder-mode',
Expand Down