feat: tag search or filter in explorer #1079
Conversation
This reverts commit 8bf3da5.
Codecov Report
@@ Coverage Diff @@
## main #1079 +/- ##
==========================================
- Coverage 85.13% 85.10% -0.03%
==========================================
Files 821 822 +1
Lines 16993 17007 +14
Branches 2056 2056
==========================================
+ Hits 14467 14474 +7
- Misses 2495 2502 +7
Partials 31 31
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
|
@aaron-steinfeld @anandtiwary this is related to #1075 |
projects/components/src/filtering/filter/builder/types/abstract-filter-builder.ts
Outdated
Show resolved
Hide resolved
So the reason behind that is, Suppose if go with the approach of doing that then my suggestion would be to extend that component to take different cc: @aaron-steinfeld |
We certainly don't want the circular dependency, but my suggestion of using contents or templates passed in account for that. That's generally how you get component libraries to be flexible - rather than encoding behavior in the common components, you allow the flexibility for the parent to do it. For example, the table works this way - defined commonly with many specific renderers the table itself doesn't know about. So using it could look like (pseudo): <list>
<ng-container *listKeyRenderer="let key">
{{key}}
</ng-container>
<ng-container *listValueRenderer="let value">
<my-fancy-thing-here></my-fancy-thing-here>
</ng-container>
<list> |
Okay, I got it. I am implementing the content to pass from parent, now the same thing is happening. This
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@aaron-steinfeld I have changed the approach and used the existing features as well as the approach of passing template. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -31,6 +33,9 @@ export class ListViewComponent { | |||
|
|
|||
| @Input() | |||
| public records?: ListViewRecord[]; | |||
|
|
|||
| @Input() | |||
| public action?: TemplateRef<unknown>; | |||
There was a problem hiding this comment.
We're getting close. This should work, but my suspicion is we'd get a lot more flexibility out of making this renderer the whole value renderer rather than always printing out the value as plaintext the following it with an optional additional renderer. A full value renderer can handle any use case this could, but also could do things like making the actual value itself the link - and IMO have a much more intuitive api.
The difference is code like:
<ht-list-view [records]="tagRecords" [action]="tagAction" data-sensitive-pii></ht-list-view>
<ng-template #tagAction let-tag>
<ht-explore-filter-link
*ngIf="this.getExploreNavigationParams(tag) | async as exploreNavigationParams"
[paramsOrUrl]="exploreNavigationParams"
htTooltip="See traces in Explorer"
>
</ht-explore-filter-link
></ng-template>vs code like (this would render a bit differently, depends exactly where we want value - but it shows the flexibility).
<ht-list-view [records]="tagRecords" data-sensitive-pii>
<div *htListViewValueRenderer="let value">
<ht-explore-filter-link
*ngIf="this.getExploreNavigationParams(tag) | async as exploreNavigationParams"
[paramsOrUrl]="exploreNavigationParams"
htTooltip="See traces in Explorer">
{{ value }}
</ht-explore-filter-link>
</div>
</ht-list-view>There was a problem hiding this comment.
So, this action feature, is on a row level, not on the value level. So this design of having listviewvaluerenderer, I am not so sure about this. tagRecords already contains the value. This would have made more impact if we were going to have, different type of renderers for the both the columns (for key and value) like we have for the table. As far as I understand, list view has a sole purpose of showing the text value, right?
@aaron-steinfeld
There was a problem hiding this comment.
So I missed the video before and slightly misunderstood what was happening here (I thought the action was going inside the value column), but I think the point still holds. Even though you're making the action a sibling of value, thinking of it as part of the value is a generalized version with strictly more functionality. It takes away the assumption that the key and value have to be plaintext, so I could do something like this
<ht-list-view [records]="tagRecords" data-sensitive-pii>
<div *htListViewValueRenderer="let value">
{{ value | prettyPrint }}
</div>
</ht-list-view>So, this action feature, is on a row level, not on the value level. So this design of having listviewvaluerenderer, I am not so sure about this. tagRecords already contains the value.
Either one kind of works the same. I had used the value rather than the whole record because I thought that matched yours, but since it doesn't, changing the implicit binding to record is identical from the caller's perspective:
<div *htListViewValueRenderer="let record">
{{ record.value | prettyPrint }}
</div>And could also be given named bindings (similar to say, ngFor) like:
<div *htListViewValueRenderer="let record; let key = key; let value = value;">
{{ record.value | prettyPrint }}
</div>This would have made more impact if we were going to have, different type of renderers for the both the columns (for key and value) like we have for the table. As far as I understand, list view has a sole purpose of showing the text value, right?
I left the key renderer out of this because we didn't need it, but we could eventually add it. Rather than having the sole purpose of showing plaintext, I think the list's real difference from table is the lack of interactions - it doesn't have variable columns, sorting, paging - any of that stuff.
This is how a lot of the library components out there work - with some variation depending on use case. For material table, a cell is defined in a similar way - the main difference is that for mat table there's a generic cellDef directive assigned to a column based on dom location, while we can just enumerate two different directives since we know there's exactly two columns.
<ng-container matColumnDef="position">
<th mat-header-cell *matHeaderCellDef> No. </th>
<td mat-cell *matCellDef="let element"> {{element.position}} </td>
</ng-container>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| </div> | ||
| <ng-template #defaultValueRenderer let-record |
There was a problem hiding this comment.
this is inside the ng for, but we only need the template defined once - it can move up to the top level.
| @@ -14,7 +15,7 @@ describe('Span detail component', () => { | |||
| component: SpanDetailComponent, | |||
| imports: [SpanDetailModule, HttpClientTestingModule, IconLibraryTestingModule], | |||
| declareComponent: false, | |||
| providers: [mockProvider(NavigationService)] | |||
| providers: [mockProvider(NavigationService), mockProvider(ExplorerService)] | |||
There was a problem hiding this comment.
This test is failing due to its transitive dependencies on router, due to the new usage of a link component. It's tech debt that this test isn't shallow, so let's fix that :)
diff --git a/projects/observability/src/shared/components/span-detail/span-detail.component.test.ts b/projects/observability/src/shared/components/span-detail/span-detail.component.test.ts
index a169698..22b8fb7 100644
--- a/projects/observability/src/shared/components/span-detail/span-detail.component.test.ts
+++ b/projects/observability/src/shared/components/span-detail/span-detail.component.test.ts
@@ -1,21 +1,14 @@
-import { HttpClientTestingModule } from '@angular/common/http/testing';
import { fakeAsync } from '@angular/core/testing';
-import { IconLibraryTestingModule } from '@hypertrace/assets-library';
-import { NavigationService } from '@hypertrace/common';
-import { createHostFactory, mockProvider, Spectator } from '@ngneat/spectator/jest';
-import { ExplorerService } from '../../../pages/explorer/explorer-service';
+import { createHostFactory, Spectator } from '@ngneat/spectator/jest';
import { SpanData } from './span-data';
import { SpanDetailComponent } from './span-detail.component';
-import { SpanDetailModule } from './span-detail.module';
describe('Span detail component', () => {
let spectator: Spectator<SpanDetailComponent>;
const createHost = createHostFactory({
component: SpanDetailComponent,
- imports: [SpanDetailModule, HttpClientTestingModule, IconLibraryTestingModule],
- declareComponent: false,
- providers: [mockProvider(NavigationService), mockProvider(ExplorerService)]
+ shallow: true
});
test('should display child components', () => {
This comment has been minimized.
This comment has been minimized.
Screen.Recording.2021-08-30.at.3.42.39.PM.mov