Skip to content

feat: adding server side search support #723

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 7 commits into from
Mar 26, 2021
Merged

feat: adding server side search support #723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d68e43
feat: adding server side search support
anandtiwary Mar 25, 2021
226d131
refactor: addressing review comments
anandtiwary Mar 25, 2021
5ce64a7
refactor: remove hiding search on options length logic
anandtiwary Mar 25, 2021
ee0f4de
Merge branch 'main' into multi-select-support-server-search
anandtiwary Mar 26, 2021
d00685c
refactor: addressing review comments
anandtiwary Mar 26, 2021
05a6b70
Merge branch 'multi-select-support-server-search' of github.com:hyper…
anandtiwary Mar 26, 2021
065255e
refactor: update comment
anandtiwary Mar 26, 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
82 changes: 69 additions & 13 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
@@ -1,24 +1,25 @@
import { CommonModule } from '@angular/common';
import { fakeAsync, flush } from '@angular/core/testing';
import { IconType } from '@hypertrace/assets-library';
import { IconLibraryTestingModule, IconType } from '@hypertrace/assets-library';
import { NavigationService } from '@hypertrace/common';
import { createHostFactory, mockProvider, SpectatorHost } from '@ngneat/spectator/jest';
import { MockComponent } from 'ng-mocks';
import { NEVER } from 'rxjs';
import { ButtonComponent } from '../button/button.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';
import { MultiSelectJustify } from './multi-select-justify';
import { MultiSelectComponent, TriggerLabelDisplayMode } from './multi-select.component';
import { MultiSelectComponent, MultiSelectSearchMode, TriggerLabelDisplayMode } from './multi-select.component';

describe('Multi Select Component', () => {
const hostFactory = createHostFactory<MultiSelectComponent<string>>({
component: MultiSelectComponent,
imports: [PopoverModule, CommonModule],
imports: [PopoverModule, CommonModule, LoadAsyncModule, IconLibraryTestingModule],
providers: [
mockProvider(NavigationService, {
navigation$: NEVER
Expand Down Expand Up @@ -48,14 +49,15 @@ describe('Multi Select Component', () => {
test('should display initial selections', fakeAsync(() => {
spectator = hostFactory(
`
<ht-multi-select [selected]="selected" [triggerLabelDisplayMode]="triggerLabelDisplayMode" [enableSearch]="true">
<ht-multi-select [selected]="selected" [triggerLabelDisplayMode]="triggerLabelDisplayMode" [searchMode]="searchMode">
<ht-select-option *ngFor="let option of options" [label]="option.label" [value]="option.value">
</ht-select-option>
</ht-multi-select>`,
{
hostProps: {
options: selectionOptions,
selected: [selectionOptions[1].value],
searchMode: MultiSelectSearchMode.CaseInsensitive,
triggerLabelDisplayMode: TriggerLabelDisplayMode.Selection
}
}
Expand Down Expand Up @@ -187,7 +189,7 @@ describe('Multi Select Component', () => {

spectator = hostFactory(
`
<ht-multi-select [selected]="selected" (selectedChange)="onChange($event)" [placeholder]="placeholder" [enableSearch]="enableSearch">
<ht-multi-select [selected]="selected" (selectedChange)="onChange($event)" [placeholder]="placeholder" [searchMode]="searchMode">
<ht-select-option *ngFor="let option of options" [label]="option.label" [value]="option.value">
</ht-select-option>
</ht-multi-select>`,
Expand All @@ -196,7 +198,7 @@ describe('Multi Select Component', () => {
options: selectionOptions,
selected: [selectionOptions[1].value],
placeholder: 'Select options',
enableSearch: true,
searchMode: MultiSelectSearchMode.CaseInsensitive,
onChange: onChange
}
}
Expand Down Expand Up @@ -231,7 +233,7 @@ describe('Multi Select Component', () => {
expect(spectator.query(LabelComponent)?.label).toEqual('first and 5 more');

spectator.setHostInput({
enableSearch: false
searchMode: MultiSelectSearchMode.Disabled
});

expect(spectator.query('.search-bar', { root: true })).not.toExist();
Expand Down Expand Up @@ -313,16 +315,19 @@ describe('Multi Select Component', () => {
}));

test('should show searchbox if applicable and function as expected', fakeAsync(() => {
const onSearchValueChangeSpy = jest.fn();

spectator = hostFactory(
`
<ht-multi-select [enableSearch]="enableSearch">
<ht-multi-select [searchMode]="searchMode" (searchValueChange)="onSearchValueChange($event)">
<ht-select-option *ngFor="let option of options" [label]="option.label" [value]="option.value">
</ht-select-option>
</ht-multi-select>`,
{
hostProps: {
options: selectionOptions,
enableSearch: true
searchMode: MultiSelectSearchMode.CaseInsensitive,
onSearchValueChange: onSearchValueChangeSpy
}
}
);
Expand All @@ -334,13 +339,15 @@ describe('Multi Select Component', () => {

spectator.component.searchOptions('fi');
spectator.tick();
expect(onSearchValueChangeSpy).toHaveBeenLastCalledWith('fi');

let options = spectator.queryAll('.multi-select-option', { root: true });
expect(options.length).toBe(2);
expect(options[0]).toContainText('first');

spectator.component.searchOptions('i');
spectator.tick();
expect(onSearchValueChangeSpy).toHaveBeenLastCalledWith('i');

options = spectator.queryAll('.multi-select-option', { root: true });
expect(options.length).toBe(4);
Expand All @@ -351,15 +358,64 @@ describe('Multi Select Component', () => {
expect(spectator.query('.clear-selected', { root: true })).not.toExist(); // Due to initial selection
expect(spectator.query('.select-all', { root: true })).toExist();

// Set selected options to less than 5 and search box and buttons should hide
// Set options list to less than 1 and search control buttons should be hidden
spectator.setHostInput({
options: selectionOptions.slice(0, 3)
options: []
});

expect(spectator.query('.search-bar', { root: true })).not.toExist();
expect(spectator.query('.divider', { root: true })).not.toExist();
expect(spectator.query('.search-bar', { root: true })).toExist();
expect(spectator.query('.divider', { root: true })).toExist();
expect(spectator.query('.clear-selected', { root: true })).not.toExist();
expect(spectator.query('.select-all', { root: true })).not.toExist();
flush();
}));

test('should show search box and emit only when search mode is emit only', fakeAsync(() => {
const onSearchValueChangeSpy = jest.fn();

spectator = hostFactory(
`
<ht-multi-select [searchMode]="searchMode" (searchValueChange)="onSearchValueChange($event)">
<ht-select-option *ngFor="let option of options" [label]="option.label" [value]="option.value">
</ht-select-option>
</ht-multi-select>`,
{
hostProps: {
options: selectionOptions,
searchMode: MultiSelectSearchMode.EmitOnly,
onSearchValueChange: onSearchValueChangeSpy
}
}
);

spectator.click('.trigger-content');

const searchBar = spectator.query('.search-bar', { root: true });
expect(searchBar).toExist();

spectator.component.searchOptions('fi');
spectator.tick();
expect(onSearchValueChangeSpy).toHaveBeenLastCalledWith('fi');

// No change in options length since for this test, externally we did not filter any option
let options = spectator.queryAll('.multi-select-option', { root: true });
expect(options.length).toBe(6);
spectator.component.searchOptions('i');
spectator.tick();
expect(onSearchValueChangeSpy).toHaveBeenLastCalledWith('i');

options = spectator.queryAll('.multi-select-option', { root: true });
expect(options.length).toBe(6);

// Set selected options to less than 5 and search box and buttons should still be visible
spectator.setHostInput({
options: selectionOptions.slice(0, 3)
});

expect(spectator.query('.search-bar', { root: true })).toExist();
expect(spectator.query('.divider', { root: true })).toExist();
expect(spectator.query('.clear-selected', { root: true })).not.toExist();
expect(spectator.query('.select-all', { root: true })).toExist();
flush();
}));
});
79 changes: 47 additions & 32 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 @@ -56,41 +56,39 @@ import { MultiSelectJustify } from './multi-select-justify';
</ht-popover-trigger>
<ht-popover-content>
<div class="multi-select-content" [ngStyle]="{ 'min-width.px': triggerContainer.offsetWidth }">
<ng-container *ngIf="this.enableSearch">
<ng-container *ngIf="this.searchMode !== '${MultiSelectSearchMode.Disabled}'">
<ng-container *ngIf="this.allOptions$ | async as allOptions">
<ng-container *ngIf="allOptions.length > 5">
<ht-search-box
class="search-bar"
(valueChange)="this.searchOptions($event)"
[debounceTime]="200"
displayMode="${SearchBoxDisplayMode.NoBorder}"
></ht-search-box>
<ht-divider class="divider"></ht-divider>

<ht-button
class="clear-selected"
*ngIf="this.isAnyOptionSelected()"
role="${ButtonRole.Primary}"
display="${ButtonStyle.Text}"
label="Clear Selected"
(click)="this.onClearSelected()"
></ht-button>

<ht-button
class="select-all"
*ngIf="!this.isAnyOptionSelected()"
role="${ButtonRole.Primary}"
display="${ButtonStyle.Text}"
label="Select All"
(click)="this.onSelectAll()"
></ht-button>
</ng-container>
<ht-search-box
class="search-bar"
(valueChange)="this.searchOptions($event)"
[debounceTime]="200"
displayMode="${SearchBoxDisplayMode.NoBorder}"
></ht-search-box>
<ht-divider class="divider"></ht-divider>

<ht-button
class="clear-selected"
*ngIf="this.isAnyOptionSelected()"
role="${ButtonRole.Primary}"
display="${ButtonStyle.Text}"
label="Clear Selected"
(click)="this.onClearSelected()"
></ht-button>

<ht-button
class="select-all"
*ngIf="allOptions.length > 0 && !this.isAnyOptionSelected()"
role="${ButtonRole.Primary}"
display="${ButtonStyle.Text}"
label="Select All"
(click)="this.onSelectAll()"
></ht-button>
</ng-container>
</ng-container>

<div class="multi-select-options">
<div class="multi-select-options" *htLoadAsync="this.filteredOptions$ as filteredOptions">
<div
*ngFor="let item of this.filteredOptions$ | async"
*ngFor="let item of filteredOptions"
(click)="this.onSelectionChange(item)"
class="multi-select-option"
>
Expand Down Expand Up @@ -134,7 +132,7 @@ export class MultiSelectComponent<V> implements AfterContentInit, OnChanges {
public showBorder: boolean = false;

@Input()
public enableSearch: boolean = false;
public searchMode: MultiSelectSearchMode = MultiSelectSearchMode.Disabled;

@Input()
public justify: MultiSelectJustify = MultiSelectJustify.Left;
Expand All @@ -145,6 +143,9 @@ export class MultiSelectComponent<V> implements AfterContentInit, OnChanges {
@Output()
public readonly selectedChange: EventEmitter<V[]> = new EventEmitter<V[]>();

@Output()
public readonly searchValueChange: EventEmitter<string> = new EventEmitter<string>();

@ContentChildren(SelectOptionComponent)
private readonly allOptionsList?: QueryList<SelectOptionComponent<V>>;
public allOptions$!: Observable<QueryList<SelectOptionComponent<V>>>;
Expand All @@ -170,7 +171,15 @@ export class MultiSelectComponent<V> implements AfterContentInit, OnChanges {
}

public searchOptions(searchText: string): void {
this.searchSubject.next(searchText);
if (this.searchMode === MultiSelectSearchMode.Disabled) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

this case should still emit. There's no harm in doing so - it could be useful for a component's consumer to know when the search has changed. I'd expect that from a lib component and here it's actually more complex to avoid. Also, it says so in the enum comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed the comment. I think this can be confusing. Until there is a known use case, I think we should not emit for this mode since filtering is applied internally. That's why i was keeping Implicit as it's name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer calling it Implicit for now and keep the emit logic as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

What part do you find confusing? A component is expected to emit when its state changes - the caller decides whether they care or not.

The name implicit really doesn't indicate anything to the caller - it just says that something is not directly expressed. But since we're directly expressing that, why not have it describe what it's actually doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If search is happening implicitly then why emit on the search value. As a consumer am I supposed to act on it? The name case-insensitive doesn't convey well that there is a auto filtering applied

Copy link
Contributor Author

@anandtiwary anandtiwary Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I am fine with that name, but with that name I don't think we should emit. Can we keep the same name but add the emit logic after we have a valid use case?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I'm caught up on this is that it's not that we're omitting something until we need it - that, I'm pretty much always for. Here, we're adding extra logic to make the behavior of an output inconsistent until we need to use it. In that case, I'd much rather have consistency even if we're not always using it. Outputs are meant to tell you about a state change, in this case, it's telling us that the search string has changed - the host is the one deciding whether to do anything with that or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh come on :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

Choose a reason for hiding this comment

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

you're my hero!

}

if (this.searchMode === MultiSelectSearchMode.CaseInsensitive) {
this.searchSubject.next(searchText);
}

this.searchValueChange.emit(searchText);
}

public onSelectAll(): void {
Expand Down Expand Up @@ -233,3 +242,9 @@ export const enum TriggerLabelDisplayMode {
Selection = 'selection-mode',
Icon = 'icon-mode'
}

export const enum MultiSelectSearchMode {
Disabled = 'disabled', // Search is not available
CaseInsensitive = 'case-insensitive', // Current available values are filtered in a case insensitive way and an emit is triggered
EmitOnly = 'emit-only' // Current available values not filtered, but an emit still triggered
}
12 changes: 11 additions & 1 deletion projects/components/src/multi-select/multi-select.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,22 @@ import { ButtonModule } from '../button/button.module';
import { DividerModule } from '../divider/divider.module';
import { IconModule } from '../icon/icon.module';
import { LabelModule } from '../label/label.module';
import { LoadAsyncModule } from '../load-async/load-async.module';
import { PopoverModule } from '../popover/popover.module';
import { TraceSearchBoxModule } from '../search-box/search-box.module';
import { MultiSelectComponent } from './multi-select.component';

@NgModule({
imports: [CommonModule, IconModule, LabelModule, PopoverModule, DividerModule, TraceSearchBoxModule, ButtonModule],
imports: [
CommonModule,
IconModule,
LabelModule,
PopoverModule,
DividerModule,
TraceSearchBoxModule,
ButtonModule,
LoadAsyncModule
],
declarations: [MultiSelectComponent],
exports: [MultiSelectComponent]
})
Expand Down