-
Notifications
You must be signed in to change notification settings - Fork 33
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: adding units to the gauge #580
base: master
Are you sure you want to change the base?
Conversation
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.
-
Selecting 'time' or 'usd' causes the gauge to render an "undefined" below the right side of the gauge.
-
Selecting any unit (except 'none') cause the Gauge Lines option not to work. The gauge no longer renders the correct number of lines.
It seems like the pre-defined units have a fixed scale, and the scale is exponential. For example, time goes from seconds all the way to 30 days within the same gauge, with each evenly spaced section jumping from 1 minute -> 1 hour -> 12 hours -> 24 hours -> 30 days. Wouldn't this cause the gauge to stay in a fixed position if my data set has only values between, say, 0 minutes to 100 minutes? The gauge would hardly move at all and it would be hard to distinguish between the low end versus the high end values. And what happens if my data set goes beyond 30 days? Seems like the gauge would not be able to render it. |
} else if (i / (lineCount + 1) >= 0.5) { | ||
ctx.textAlign = 'left' | ||
if (gaugeUnit.toString() === 'bytes') { | ||
const labels = ['0b', '1024Kb', '1024Mb', '1024Gb', '1024Tb', '1024Pb'] |
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.
I don't think this is the most useful way to do units.
ctx.rotate(-startDegree) | ||
} | ||
} else if (gaugeUnit.toString() === 'USD') { | ||
const labels = ['0', '100', '1000', '1000m', '1000b', '1000t'] |
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.
I don't think this is the most useful way to do units.
ctx.rotate(-startDegree) | ||
} | ||
} else if (gaugeUnit.toString() === 'time') { | ||
const labels = ['0', '60s', '60m', '12h', '24h', '30d'] |
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.
I don't think this is the most useful way to do units.
This is the first step for issue 19880, this is the giraffe addition that needs to now be added to UI. Which is when i will close that issue.
But it does close this issue #343. Only thing to note is this does only add this functionality to gauge. If we want to add it to other graph types we need to create a separate issue for that.