Explorer - Fix Retaining of filters on switching between Trace and Span view #1486
Conversation
…fallback to current logic
…2 way binding for filters
jaywalker21
changed the title
| public onContextUpdated(contextWrapper: ExplorerContextScope): void { | ||
| this.attributes$ = this.metadataService.getFilterAttributes(contextWrapper.dashboardContext); | ||
| const listener = this.attributes$.subscribe(attributes => { |
There was a problem hiding this comment.
Will try to move this block outside of this method in an independent structure.
Codecov Report
@@ Coverage Diff @@
## main #1486 +/- ##
===========================================
- Coverage 84.04% 59.10% -24.95%
===========================================
Files 858 858
Lines 18296 18328 +32
Branches 2481 2493 +12
===========================================
- Hits 15377 10832 -4545
- Misses 2802 7163 +4361
- Partials 117 333 +216
Continue to review full report at Codecov.
|
| @@ -327,6 +371,12 @@ interface ExplorerContextScope { | |||
| scopeQueryParam: ScopeQueryParam; | |||
| } | |||
|
|
|||
| type contextMap = { | |||
There was a problem hiding this comment.
PascalCase for type names (also, we generally use interfaces if constructing a brand new type that's not projected from another.
Naming here is also an issue. I think context is an overloaded term, and even knowing the goal of this PR it took me reading the object using this to understand what this was. Maybe something like
type AttributeTranslationDictionary = { [key: string]: string; }
const spanToApiTraceAttributeTranslationDictionary = {...};
const apiTraceToSpanAttributeTranslationDictionary = {...};| public onContextUpdated(contextWrapper: ExplorerContextScope): void { | ||
| this.attributes$ = this.metadataService.getFilterAttributes(contextWrapper.dashboardContext); | ||
| const listener = this.attributes$.subscribe(attributes => { |
There was a problem hiding this comment.
We shouldn't use a subscription here. Make the filters an observable, and subscribe via async pipe in the template.
| const newFilters = this.filters.map(eachFilter => { | ||
| // If the given filter has a different name for the selected tab, update the filter value | ||
| if (eachFilter.field in contextMapObject[lastTab]) { | ||
| const newFilter = this.filterChipService.autocompleteFilters( |
There was a problem hiding this comment.
We should be able to do this without the chip service, that's an impl detail from the filter bar. Can we use the filter builder service (I hate how complex we made this!) instead just read the details out of the old one and write them into the new one. May need to lookup the new attribute, too - but we shouldn't have to create it, it's already one of the ones in the array coming in.
| this.onFiltersChanged(this.filterBarService.addFilter(this.internalFiltersSubject$.value, filter)); | ||
| this.onFiltersChanged( | ||
| this.filterBarService.addFilter( | ||
| isEmpty(this.internalFiltersSubject$.value) ? [] : this.internalFiltersSubject$.value, |
There was a problem hiding this comment.
value should never be undefined, so we're just checking if empty and if so assigning empty? did some logic change that forced this? Either this isn't required or we've broken the assigned type.
| @@ -175,10 +193,10 @@ export class FilterBarComponent implements OnChanges, OnInit, OnDestroy { | |||
| } | |||
|
|
|||
| private updateFilter(oldFilter: Filter, newFilter: Filter): void { | |||
| this.onFiltersChanged(this.filterBarService.updateFilter(this.internalFiltersSubject$.value, oldFilter, newFilter)); | |||
| this.onFiltersChanged(this.filterBarService.updateFilter(this.filters || [], oldFilter, newFilter)); | |||
There was a problem hiding this comment.
When do we use the subject's value and when do we use this.filters? We seem to use different ways in different places which makes it very confusion to understand as a reader.
Description
Fixes #1250
filter-barcomponent controlled in nature by providing a mechanism for two-way binding.explorercomponent to update the filter values on toggling betweenspanandtraceviews.Testing