fix: fix misc issues with gauge and donuts #950
Conversation
Codecov Report
@@ Coverage Diff @@
## main #950 +/- ##
==========================================
- Coverage 85.05% 85.04% -0.01%
==========================================
Files 812 812
Lines 16723 16737 +14
Branches 2004 2010 +6
==========================================
+ Hits 14223 14234 +11
- Misses 2468 2472 +4
+ Partials 32 31 -1
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…race-ui into dashboard-misc-changes
This comment has been minimized.
This comment has been minimized.
| @@ -126,19 +126,7 @@ export class DonutBuilderService extends D3VisualizationBuilderService< | |||
| } | |||
|
|
|||
| protected decorateDimensions(calculatedDimensions: ChartDimensions): DonutDimensions { | |||
| let diameter = Math.min(calculatedDimensions.visualizationWidth, calculatedDimensions.visualizationHeight); | |||
There was a problem hiding this comment.
@skjindal93 @aaron-steinfeld Do you guys remember why we need this logic? It seems to reduce the size of donut by a lot. It creates problem with smaller container size.
After I removed it, I didn't notice any issues.
There was a problem hiding this comment.
Basically, we wanted the smaller dimension to decide the diameter for the donut. If the screen has more width than the height, the height determines the diameter and vice versa
If there is a way to automatically fit in the donut, irrespective of different screen sizes, then that would be perfect
There was a problem hiding this comment.
Yeah, this logic was for sizing, based on the original donut mocks way back. The comments explain it, but it's mostly about legend, padding and fitting a square visualization into potentially rectangular bounding boxes like @skjindal93 mentioned.
There was a problem hiding this comment.
okay, so with the new changes, the Donut is still working fine. It is however making a better use of the space and appearing bigger. Apart from that, I am not seeing any failure in terms of rendering. We might want to reduce the size of the dashboard containers though so that it can match previous size.
There was a problem hiding this comment.
I'm a little hesitant about this change, mainly wrt the legend. Two things are being changed here.
- We're not assigning legend width (why not? are we not using a legend?). Does legend still work if rendered below? to the side?
- We're not using the configured padding. This is fine if that's the intended visual change, but if so, any supporting code like that constant should be removed. That said, I'd guess that would show up as the legend sitting right next to the chart with 0 gap, which will look broken too.
The logic looks like it's doing these things regardless of legend position (If I vaguely recall, I think when written we always showed right legends) so that should be fixed, but this reads to me like we changed an assumption of a legend into an assumption of no legend, which is equally wrong.
There was a problem hiding this comment.
we are already using Flex in container. So css is automatically assigning them correct width and position. The padding also appears to be fine visually.
There was a problem hiding this comment.
Let me share the screenshot in slack
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…race-ui into dashboard-misc-changes
This comment has been minimized.
This comment has been minimized.
| private static readonly DONUT_PADDING_PX: number = 60; | ||
| private static readonly LEGEND_SIZE_MULTIPLE: number = 1.5; | ||
| public static readonly DONUT_PADDING_PX: number = 10; | ||
| public static readonly LEGEND_SIZE_MULTIPLE: number = 1.5; |
There was a problem hiding this comment.
can you remove the unused ones and revert any still used to private
| @@ -15,3 +15,4 @@ browserslist | |||
| LICENSE* | |||
| conf/ | |||
| coverage/ | |||
| test-results/ | |||
There was a problem hiding this comment.
Thank you! Been meaning to do this for... months.
This comment has been minimized.
This comment has been minimized.
Unit Test Results 4 files 259 suites 15m 47s ⏱️ Results for commit dea7932. |
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.