-
Notifications
You must be signed in to change notification settings - Fork 26
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
ref: Render only two levels of the sunburst #3783
ref: Render only two levels of the sunburst #3783
Conversation
Bundle ReportChanges will decrease total bundle size by 33.49kB (-0.27%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-staging-esmAssets Changed:
Files in
view changes for bundle: gazebo-staging-systemAssets Changed:
Files in
|
Bundle ReportChanges will decrease total bundle size by 33.49kB (-0.27%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-production-esmAssets Changed:
Files in
view changes for bundle: gazebo-production-systemAssets Changed:
Files in
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3783 +/- ##
=======================================
Coverage 98.74% 98.74%
=======================================
Files 826 826
Lines 14947 14947
Branches 4256 4264 +8
=======================================
Hits 14759 14759
Misses 181 181
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3783 +/- ##
=======================================
Coverage 98.74% 98.74%
=======================================
Files 826 826
Lines 14947 14947
Branches 4256 4256
=======================================
Hits 14759 14759
Misses 181 181
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3783 +/- ##
=======================================
Coverage 98.74% 98.74%
=======================================
Files 826 826
Lines 14947 14947
Branches 4256 4256
=======================================
Hits 14759 14759
Misses 181 181
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3783 +/- ##
=======================================
Coverage 98.74% 98.74%
=======================================
Files 826 826
Lines 14947 14947
Branches 4264 4256 -8
=======================================
Hits 14759 14759
Misses 181 181
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
6fc34b6
to
a0b9798
Compare
.clamp(true) | ||
}) | ||
function createColorFunction(parentSpan) { | ||
return Sentry.startSpan({ name: 'SunburstChart.color', parentSpan }, () => |
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.
do we use yaml set color indication range for this? seems like no
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 do not no, it would be really funky with the interpolation and how we handle it
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.
Gotcha okay
I feel like if I click on a leaf node, it should open the file. This is already a feature but I have to go click the link underneath the sunburst. Thoughts? Other than that, I'm not qualified to scrutinize your d3 usage, but code looks good and seems to work as expected, so I'll approve. |
I like the idea, but it's a bit beyond the scope of this effort. Can you make up a ticket, and we can look at adding that in down the line? The breadcrumb where you are clicking now, lives outside of the Sunburst chart, so would also require a tad of exploring to figure out what exactly to do. |
Description
The goal of this PR is to optimize the sunbursts performance overall and reduce the lagginess when viewing the coverage overview page. This was accomplished by changing the way that we render the sunburst. Previously we would render the entire tree structure of the repo, hiding elements that weren't currently visible. This lead to many performance issues as we were creating two elements per file in a repo. When users interacted with the sunburst, under the hood it was actually D3 updating the elements attributes overtime "animating" the transition.
With this refactor we have changed our rendering strategy, and how users interact with the sunburst:
Initial render:
renderArcs
..
)On Click
renderArcs
One regression of this change is the removal of the animation with the sunburst, as the change to the render process doesn't support this. We may explore some potential solutions for bringing this back down the road, however for the time being we will be leaving it as is.
Closes: codecov/engineering-team#3382
Notable Changes