-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(trace-explorer): Render list of trace related project icons #73691
feat(trace-explorer): Render list of trace related project icons #73691
Conversation
Instead of always rendering a single project icon for the trace root, render a list of project icons for all related projects in the trace. We try to prioritize projects that are selected in order to avoid users thinking it's making a cross project query.
maxVisibleProjects?: number; | ||
} | ||
|
||
export function ProjectsRenderer({ |
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.
This was largely adapted from AvatarList
however that component is very much specialized right now and doesn't extend to this use case very well. A good place to clean up in the future.
Bundle ReportChanges will increase total bundle size by 2.82kB ⬆️
|
…render-list-of-trace-related-project-icons
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.
Nothing breaking, a few code quality comments though.
grid-template-columns: 116px auto repeat(2, min-content) 85px 112px 66px; | ||
`; | ||
|
||
const SpanPanelContent = styled('div')` | ||
width: 100%; | ||
display: grid; | ||
grid-template-columns: repeat(1, min-content) auto repeat(1, min-content) 160px 85px; | ||
grid-template-columns: 100px auto repeat(1, min-content) 160px 85px; |
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.
Why? doesn't this cover ids?
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.
we dont use a monospace font for the id so depending on the id, the trace/span id could end up with different widths using min-content
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.
Ah now you say it I did notice that
…render-list-of-trace-related-project-icons
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #73691 +/- ##
==========================================
+ Coverage 78.11% 78.12% +0.01%
==========================================
Files 6659 6659
Lines 297962 298009 +47
Branches 51266 51270 +4
==========================================
+ Hits 232752 232825 +73
+ Misses 58920 58895 -25
+ Partials 6290 6289 -1
|
Instead of always rendering a single project icon for the trace root, render a list of project icons for all related projects in the trace. We try to prioritize projects that are selected in order to avoid users thinking it's making a cross project query.
Screenshot