feat: log records in a new tab#916
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #916 +/- ##
==========================================
+ Coverage 85.14% 85.21% +0.07%
==========================================
Files 806 812 +6
Lines 16644 16714 +70
Branches 2098 2003 -95
==========================================
+ Hits 14171 14243 +72
+ Misses 2441 2439 -2
Partials 32 32
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| @Component({ | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| template: ` | ||
| <ng-container *ngIf="this.logEvents$ | async as logEvents"> |
There was a problem hiding this comment.
If we are fetching from graphql then we should use *htLoadAsync so that we can show the loader icon
There was a problem hiding this comment.
Actually, for this
Data is already being fetched before this, on the page level. and here it will be only used as a static data observable (shareReplay)
There was a problem hiding this comment.
okay. I hope that doesn't change :)
There was a problem hiding this comment.
It is better to use loadAsync. If in case fetchLogEvents starts returning a new observable, we would have to remember to update this template as well.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| type: SpanType.Exit, | ||
| spanTags: {} | ||
| spanTags: {}, | ||
| logEvents: [] |
There was a problem hiding this comment.
We could add properties so that the test becomes relevant :)
anandtiwary
left a comment
There was a problem hiding this comment.
Looks good. Please test the two pages in ht-ui and t-ui before merging.
This comment has been minimized.
This comment has been minimized.
|
Some suggestions based on offline conversation (haven't reviewed most of PR, will leave that to others): #935 |
This comment has been minimized.
This comment has been minimized.
| </ng-container> | ||
| ` | ||
| }) | ||
| export class TraceLogsComponent { |
There was a problem hiding this comment.
So this is not a dash page. It works. But I am not sure if we should be diverge from the dashboard pattern.
@aaron-steinfeld thoughts?
There was a problem hiding this comment.
IMO, we should try to reign dashboards back in to pages that look like dashboards. If we later need to embed this as a widget in another page, we could widgetize it for sharing at that point - but no need to build these custom single-page, single-purpose widgets.
Description
log records in a new tab
Testing
Local testing done!!
Checklist: