Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

agzuniverse
Copy link

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

This improves the readability of the bars significantly.
@agzuniverse agzuniverse marked this pull request as ready for review March 11, 2020 19:52
@agzuniverse
Copy link
Author

@kortschak you mentioned #556 cannot be merged because it does not pass the CI build, so here's a PR which does.

Copy link
Member

@kortschak kortschak left a 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.


// ShowLabel determines whether the value of the bars should be
// shown above it or not.
ShowLabel bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShowValue?

Copy link
Author

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.

@xiaofeng-zerone
Copy link

When can master branches use this feature?

@kortschak
Copy link
Member

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.
@agzuniverse
Copy link
Author

I'll look into adding a test for this soon.

@lynxplay
Copy link

lynxplay commented May 6, 2021

Is there any plan on moving forward with this feature ?
It seems like a really nice addition and I'd personally love to use the master branch of gonum/plot rather than my own fork just for this feature.

@agzuniverse
Copy link
Author

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.

@lynxplay
Copy link

lynxplay commented May 8, 2021

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?

@sbinet
Copy link
Member

sbinet commented Jun 24, 2021

yes, we like to have a simple testdata/foo_golden.{png,svg,...} reference file and a test (or an example) generating that file to compare with and make sure we don't introduce regressions when refactoring code.

here is an example:

(with my apologies for the belated answer.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plot: Can't add data labels to the barchart
5 participants