-
Notifications
You must be signed in to change notification settings - Fork 204
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
Show value of each bar on top of it #585
base: master
Are you sure you want to change the base?
Conversation
This improves the readability of the bars significantly.
@kortschak you mentioned #556 cannot be merged because it does not pass the CI build, so here's a PR which does. |
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 needs a test.
plotter/barchart.go
Outdated
|
||
// ShowLabel determines whether the value of the bars should be | ||
// shown above it or not. | ||
ShowLabel bool |
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.
ShowValue
?
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've changed it to ShowValue
.
When can master branches use this feature? |
It would need to be merged first, and then it will be included in the next minor release. In order to be merged it needs to pass review, which will require that the change requests are satisfied. |
The value label is displayed on top of bar charts only when the user has set this to true.
Showing the entire float64 value is not practical and overflows badly in most situations.
c2b1710
to
d5ee20b
Compare
I'll look into adding a test for this soon. |
Is there any plan on moving forward with this feature ? |
I think the only reason this PR wasn't merged is because I didn't write tests for this change. I thought of doing it but unfortunately couldn't find the time. @lynxplay if you could add a test the maintainers will probably merge this change in to master. |
Yeah I tried looking into this but the generated plots don't respect the written values on top of the bars when cropping so one value is always cropped off. Tests on this are mainly just a single generated graph and an image comparison no? |
yes, we like to have a simple here is an example:
(with my apologies for the belated answer.) |
This change modifies the bar charts to show their values as a label on top of each bar if the user sets the new
b.ShowLabel
option to true.This PR builds on #556
Closes #475