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

Bar chart labels #1031

Closed
wants to merge 6 commits into from
Closed

Bar chart labels #1031

wants to merge 6 commits into from

Conversation

nreese
Copy link

@nreese nreese commented Oct 27, 2015

pull request for issue #211 adding support for bar chart labels

//_chart.label() sets _renderLabel to true.
//Set back to false to disable renderLabel by default
_chart.renderLabel(false);

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like an odd default value, the Y coordinate? why not just leave .label() uninitialized?

@gordonwoodhull
Copy link
Contributor

I must be missing something about the label = Y coordinate default.

Other than that this looks really good. Thanks for including tests and for making it backward-compatible; since it is, we can include it in 2.0.

@nreese
Copy link
Author

nreese commented Oct 28, 2015

I figured that most users want bar labels to display the y-axis value. For my use case in particular, I had some bars that were order of magnitudes taller than others (150 vs 3). Since the small bars are so tiny, 2 or 3 pixels, I wanted a way to convey that they were not zero.
What would you recommend for a default label function?

@gordonwoodhull
Copy link
Contributor

Oh, I get it now. Yes, of course that's a reasonable default.

I guess we'll probably have to default in some floating point formatter. Your comment about tiny bars also reminds me that it would be nice to make the labels clickable.

You can take on these improvements if you want; otherwise I'll just address them when I merge.

I also see that I'll need to do something about the padding; the stacked bar example makes it painfully obvious how poorly yAxisPadding works here, because adding a gap below zero is quite wrong. It's pretty much the same change as is needed for #998 bubble chart padding. I can address this after merging this.

@nreese
Copy link
Author

nreese commented Oct 28, 2015

I can take a look at making the labels clickable - that should be straight forward.

How many decimal places should the default label generator format floating points to? Maybe 2?

What do you have in mind for y axis padding?

@gordonwoodhull
Copy link
Contributor

Thanks @nreese! We can use the same default floating point formatting that we use for the filters. It's .2f but it's configurable. In fact, maybe it makes sense to use dc.utils.printSingleValue for consistency.

https://github.com/dc-js/dc.js/blob/develop/src/utils.js

@gordonwoodhull
Copy link
Contributor

It might be enough just to have padding on each side instead of duplicated on X and Y.

But I don't want to commit to a design yet, because there is too much in dc.js which was designed hastily and continues to haunt us.

@nreese
Copy link
Author

nreese commented Oct 29, 2015

I have made the labels clickable and used dc.utils.printSingleValue for format the label value.

Let me know if anything else is needed.

@gordonwoodhull gordonwoodhull modified the milestone: v2.0 Oct 30, 2015
@nreese
Copy link
Author

nreese commented Nov 4, 2015

It looks like there is a merge conflict. How do I look at/fix the merge conflict? Are you waiting on action from me for this pull request?

@gordonwoodhull
Copy link
Contributor

Thanks @nreese, don't worry about the merge conflict, although in the future it is a best practice not to check in the build artifacts when doing a PR. (It usually doesn't matter but it makes life difficult for the maintainer in rare cases, so that's why we put that in the contribution guidelines.)

Sorry for the slow turnaround. I haven't had time to thoroughly review this. But it looks ready afaict.

@gordonwoodhull
Copy link
Contributor

This merged to 2.0 beta 21. Thanks!

@nreese, please let me know how to credit you in the AUTHORS file - your commits are marked with Reese and a bogus email address.

(Probably a good idea to set your name and your email in git if you're contributing to projects on GitHub.)

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

Successfully merging this pull request may close these issues.

2 participants