Skip to content

feat: show selected item's icon in trigger label if applicable #884

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 9 commits into from
May 28, 2021
Merged

feat: show selected item's icon in trigger label if applicable #884

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
89b8f27
feat: show selected item's icon in trigger label if applicable
arjunlalb May 27, 2021
b30f50d
fix: explicit undefined check
arjunlalb May 27, 2021
4f23fc9
fix: typedef
arjunlalb May 27, 2021
cfcbda3
fix: update logic
arjunlalb May 28, 2021
1b69a18
simplify logic. update test
arjunlalb May 28, 2021
b8f0dc3
fix: override no-null
arjunlalb May 28, 2021
3aa2e25
fix: remove unused module
arjunlalb May 28, 2021
fd47b9c
Merge branch 'main' into update-select
arjunlalb May 28, 2021
16983c3
fix: update behaviour
arjunlalb May 28, 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
52 changes: 48 additions & 4 deletions projects/components/src/select/select.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { HttpClientTestingModule } from '@angular/common/http/testing';
import { fakeAsync, flush } from '@angular/core/testing';
import { By } from '@angular/platform-browser';
import { IconLibraryTestingModule, IconType } from '@hypertrace/assets-library';
import { NavigationService } from '@hypertrace/common';
import { MemoizeModule, NavigationService } from '@hypertrace/common';
import { IconComponent } from '@hypertrace/components';
import { createHostFactory, mockProvider, SpectatorHost } from '@ngneat/spectator/jest';
import { MockComponent } from 'ng-mocks';
import { EMPTY } from 'rxjs';
import { SelectControlOptionPosition } from './select-control-option.component';
import { SelectJustify } from './select-justify';
Expand All @@ -12,8 +15,9 @@ import { SelectModule } from './select.module';
describe('Select Component', () => {
const hostFactory = createHostFactory<SelectComponent<string>>({
component: SelectComponent,
imports: [SelectModule, HttpClientTestingModule, IconLibraryTestingModule],
imports: [SelectModule, HttpClientTestingModule, IconLibraryTestingModule, MemoizeModule],
declareComponent: false,
declarations: [MockComponent(IconComponent)],
providers: [
mockProvider(NavigationService, {
navigation$: EMPTY,
Expand All @@ -27,7 +31,7 @@ describe('Select Component', () => {
const selectionOptions = [
{ label: 'first', value: 'first-value' },
{ label: 'second', value: 'second-value' },
{ label: 'third', value: 'third-value', selectedLabel: 'Third Value!!!' }
{ label: 'third', value: 'third-value', selectedLabel: 'Third Value!!!', icon: 'test-icon', iconColor: 'red' }
];

test('should display initial selection', fakeAsync(() => {
Expand Down Expand Up @@ -146,12 +150,52 @@ describe('Select Component', () => {
const optionElements = spectator.queryAll('.select-option', { root: true });
spectator.click(optionElements[2]);

expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange).toHaveBeenCalledWith(selectionOptions[2].value);
expect(spectator.query('.trigger-content')).toHaveText(selectionOptions[2].selectedLabel!);
flush();
}));

test('should set trigger-prefix-icon correctly', fakeAsync(() => {
spectator = hostFactory(
`
<ht-select [icon]="icon">
<ht-select-option *ngFor="let option of options" [label]="option.label" [value]="option.value" [icon]="option.icon" [iconColor]="option.iconColor">
</ht-select-option>
</ht-select>`,
{
hostProps: {
options: selectionOptions,
icon: 'select-level-icon'
}
}
);
spectator.tick();

// No selection -> select component level icon and no color
expect(spectator.debugElement.query(By.css('.trigger-prefix-icon')).componentInstance.icon).toBe(
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld May 28, 2021

Choose a reason for hiding this comment

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

is there a reason to use debug element querying rather than spectator querying ? is it because spectator doesn't give you access to the component on root queries?

Copy link
Contributor Author

@arjunlalb arjunlalb May 28, 2021

Choose a reason for hiding this comment

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

yep. wanted to access component to verify the contents of it.

'select-level-icon'
);
expect(spectator.debugElement.query(By.css('.trigger-prefix-icon')).componentInstance.color).toBe(undefined);

// Selection with no icon -> no icon and no color
spectator.click('.trigger-content');
let optionElements = spectator.queryAll('.select-option', { root: true });
spectator.click(optionElements[1]);
spectator.tick();
expect(spectator.query('.trigger-prefix-icon')).not.toExist();

// Selection with icon and color
spectator.click('.trigger-content');
optionElements = spectator.queryAll('.select-option', { root: true });
spectator.click(optionElements[2]);
spectator.tick();

expect(spectator.debugElement.query(By.css('.trigger-prefix-icon')).componentInstance.icon).toBe('test-icon');
expect(spectator.debugElement.query(By.css('.trigger-prefix-icon')).componentInstance.color).toBe('red');

flush();
}));

test('should set correct label alignment', fakeAsync(() => {
spectator = hostFactory(
`
Expand Down
29 changes: 28 additions & 1 deletion projects/components/src/select/select.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,13 @@ import { SelectSize } from './select-size';
class="trigger-content menu-with-border"
[ngClass]="[this.justifyClass]"
>
<ht-icon *ngIf="this.icon" class="trigger-prefix-icon" [icon]="this.icon" [size]="this.iconSize">
<ht-icon
*ngIf="this.getPrefixIcon | htMemoize: selected:this.icon"
class="trigger-prefix-icon"
[icon]="this.getPrefixIcon | htMemoize: selected:this.icon"
[size]="this.iconSize"
[color]="this.getPrefixIconColor | htMemoize: selected"
>
</ht-icon>
<ht-label class="trigger-label" [label]="selected?.selectedLabel || selected?.label || this.placeholder">
</ht-label>
Expand Down Expand Up @@ -204,6 +210,27 @@ export class SelectComponent<V> implements AfterContentInit, OnChanges {
}
}

public getPrefixIcon(
selectedOption: SelectOptionComponent<V> | undefined,
triggerIcon: string | undefined
): string | undefined {
if (selectedOption !== undefined && selectedOption?.icon !== undefined) {
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld May 27, 2021

Choose a reason for hiding this comment

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

So we're favoring the selections icon in the trigger over the trigger's icon? that doesn't seem right to me, but I guess I'd also find it strange to use a trigger icon and individual icons together

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld May 27, 2021

Choose a reason for hiding this comment

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

(also, unnecessary first condition)

Suggested change
if (selectedOption !== undefined && selectedOption?.icon !== undefined) {
if (selectedOption?.icon !== undefined) {

Copy link
Contributor Author

@arjunlalb arjunlalb May 28, 2021

Choose a reason for hiding this comment

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

if there's a selection, we should show the selection's icon or nothing.
If there's no selection, show the trigger icon. That's the intent.

It doesn't make sense to show the regular trigger icon along with a selection. that would be confusing.

we already show selected label in the trigger the same way. This is extending it to the icon as well.

Fixed the logic.

Copy link
Contributor Author

@arjunlalb arjunlalb May 28, 2021

Choose a reason for hiding this comment

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

I think my brain wasn't functioning last night when I wrote the logic for this. 🤔

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld May 28, 2021

Choose a reason for hiding this comment

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

Using the selected icon if defined makes sense, but not using the component wide icon if the selections aren't individually... iconized breaks the original intent. I think it's just two different styles:
image

in this one, that icon just is meant to apply to every option, but not appear as a duplicate in the option list.

Copy link
Contributor Author

@arjunlalb arjunlalb May 28, 2021

Choose a reason for hiding this comment

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

updated

return selectedOption.icon;
}

if (selectedOption !== undefined) {
return undefined;
}

return triggerIcon;
}

public getPrefixIconColor(selectedOption: SelectOptionComponent<V> | undefined): string | undefined {
return selectedOption !== undefined && selectedOption?.iconColor !== undefined
? selectedOption.iconColor
: undefined;
}

public ngOnChanges(changes: TypedSimpleChanges<this>): void {
if (this.items !== undefined && changes.selected !== undefined) {
this.selected$ = this.buildObservableOfSelected();
Expand Down
4 changes: 3 additions & 1 deletion projects/components/src/select/select.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { FormsModule } from '@angular/forms';
import { MemoizeModule } from '@hypertrace/common';
import { DividerModule } from '../divider/divider.module';
import { IconModule } from '../icon/icon.module';
import { LabelModule } from '../label/label.module';
Expand All @@ -21,7 +22,8 @@ import { SelectComponent } from './select.component';
LetAsyncModule,
PopoverModule,
TooltipModule,
DividerModule
DividerModule,
MemoizeModule
],
declarations: [SelectComponent, SelectOptionComponent, SelectGroupComponent, SelectControlOptionComponent],
exports: [SelectComponent, SelectOptionComponent, SelectGroupComponent, SelectControlOptionComponent]
Expand Down