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

Changes from 3 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
21 changes: 13 additions & 8 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,7 +10,7 @@ import {
QueryList
} from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { queryListAndChanges$ } from '@hypertrace/common';
import { queryListAndChanges$, SubscriptionLifecycle } from '@hypertrace/common';
import { BehaviorSubject, combineLatest, EMPTY, Observable, Subject } from 'rxjs';
import { map } from 'rxjs/operators';
import { ButtonRole, ButtonStyle } from '../button/button';
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

@aaron-steinfeld aaron-steinfeld Nov 2, 2021

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,7 +48,7 @@ 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>
<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"
Expand Down Expand Up @@ -166,6 +167,8 @@ export class MultiSelectComponent<V> implements AfterContentInit, OnChanges {
public triggerLabel?: string;
public selectedItemsCount: number = 0;

public constructor(private readonly subscriptionLifecycle: SubscriptionLifecycle) {}

public ngAfterContentInit(): void {
this.allOptions$ = this.allOptionsList !== undefined ? queryListAndChanges$(this.allOptionsList) : EMPTY;
this.filteredOptions$ = combineLatest([this.allOptions$, this.searchSubject]).pipe(
Expand Down Expand Up @@ -241,14 +244,16 @@ export class MultiSelectComponent<V> implements AfterContentInit, OnChanges {
return;
}

const selectedItems: SelectOptionComponent<V>[] | undefined = this.allOptionsList?.filter(item =>
this.isSelectedItem(item)
);
this.subscriptionLifecycle.add(
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Nov 1, 2021

Choose a reason for hiding this comment

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

It doesn't make sense to add a new subscription every time we set the trigger label. What's the bug?

Copy link
Contributor Author

@jake-bassett jake-bassett Nov 1, 2021

Choose a reason for hiding this comment

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

The bug is that the allOptionsList is not updating.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Nov 1, 2021

Choose a reason for hiding this comment

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

Ever (this would surprise me)? Or that when options update, this logic isn't rerun (this I would expect).

Copy link
Contributor Author

@jake-bassett jake-bassett Nov 1, 2021

Choose a reason for hiding this comment

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

It's not updating from the call to ngOnChanges().

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Nov 1, 2021

Choose a reason for hiding this comment

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

Right, it wouldn't - it's not coming in as an input but as content children exposed via a QueryList. That will emit when they change (and that's what allOptions is projected from). The problem is this page is a mix of async and sync behavior - the set trigger behavior is updating regular variables, the search and options are working in observables.

My suggestion would be to represent the selections as observables too. If we keep the hybrid, we're stuck managing subscriptions which is easy to mess up.

Copy link
Contributor Author

@jake-bassett jake-bassett Nov 1, 2021

Choose a reason for hiding this comment

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

I really don't think it makes sense to refactor this component right now.

Copy link
Contributor

@anandtiwary anandtiwary Nov 2, 2021

Choose a reason for hiding this comment

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

@jake-bassett What property do you want it to change on? Why should options list change when an input changes?

Copy link
Contributor

@anandtiwary anandtiwary Nov 2, 2021

Choose a reason for hiding this comment

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

I could help if you could explain the use case

Copy link
Contributor

@anandtiwary anandtiwary Nov 2, 2021

Choose a reason for hiding this comment

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

AllOptionsList will update as soon as you add/remove any select option component. Are you trying to change the properties of select option component and expecting that it would reflect in multi select?

Copy link
Contributor Author

@jake-bassett jake-bassett Nov 2, 2021

Choose a reason for hiding this comment

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

Talked a bit offline. Removed the subscription. Please take a look when you get a chance.

this.allOptions$?.subscribe(options => {
const selectedItems: SelectOptionComponent<V>[] = options.filter(item => this.isSelectedItem(item));

this.selectedItemsCount = selectedItems?.length ?? 0;
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;
// Trigger label is placeholder in case there is element selected on multiselect
this.triggerLabel = this.selectedItemsCount === 0 ? this.placeholder : selectedItems[0]?.label;
})
);
}
}

Expand Down