-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
Refactor certain filters into its own service #259
Refactor certain filters into its own service #259
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested the updated status, type, and label filters, and they are working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
this.dropdownFilter$.next(DEFAULT_DROPDOWN_FILTER); | ||
} | ||
|
||
updateSelectedLabels(newSelectedLabels: string[]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there's duplication of the logic when updating dropdownFilter itself. For instance, updateSelectedLabels
is in essence doing the same thing as updateHiddenLabels
and even the other methods: to update the right property. Instead, maybe we could follow a pattern of composition using the Partial<T>
API in TypeScript?
This could look something like this:
updateFilters(newFilters: Partial<DropdownFilter>): void {
this.dropdownFilter$.next(
...this.dropdownFilter$.value,
...newFilters,
),
}
Then after we update, we can check our rules for updating the status and type.
Consumers would then simply pass some filters that they want to change in the components themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This is a type I was not aware of, it is perfect in this situation.
I have made the appropriate changes.
@@ -18,7 +18,7 @@ | |||
</mat-form-field> | |||
<mat-form-field appearance="standard"> | |||
<mat-label>Type</mat-label> | |||
<mat-select [(value)]="this.dropdownFilter.type" (selectionChange)="updateStatusPairing(); applyDropdownFilter()"> | |||
<mat-select [value]="this.dropdownFilter.type" (selectionChange)="this.filtersService.updateType($event.value)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my earlier comment, the change here would be something like
<mat-select [value]="this.dropdownFilter.type" (selectionChange)="this.filtersService.updateFilters({type:$event.value})">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far functionality seems ok! Just a small change requested to the Filters Service :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks neat, thanks for the refactor. Let's wait for vignesh approval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥 🚀 changes look great with the addition of the pipe!
Summary:
Fixes part of #249
Type of change:
Changes Made:
Proposed Commit Message:
Checklist: