-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Introduce the 'minBarLength' dataset property for bar scales (X or Y) #5741
Conversation
I'm wondering if this should be called |
So we would have |
I would lean towards calling it |
I agree that Calling |
I agree we should switch between I didn't think that using |
I like the |
What about That being said, I think that bar specific options existing at the scale level (e.g. What do you think? |
I agree. Unless said otherwise, I'll proceed with making these adjustments and implement |
For the record, with For a vertical bar chart (on yAxes):
For an horizontal bar chart (on xAxes):
|
c7a8fd0
to
dea3705
Compare
Patch updated. Please, let me know if I need to re-create a live example out of this. |
Cool. Can you add a unit test? |
Sure. |
Please, let me know if there's anything else I could do to help pushing this new feature forward. Thanks. |
src/scales/scale.linear.js
Outdated
@@ -30,6 +30,8 @@ module.exports = function(Chart) { | |||
return isHorizontal ? meta.xAxisID === me.id : meta.yAxisID === me.id; | |||
} | |||
|
|||
me.minBarLength = opts.minBarLength; |
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 would not expose this option as a public scale member but instead directly access scale.options.minBarLength
from the bar controller as we do for other bar specific options.
src/controllers/controller.bar.js
Outdated
var datasets = chart.data.datasets; | ||
var value = scale.getRightValue(datasets[datasetIndex].data[index]); | ||
var minBarLength = scale.minBarLength; |
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.
Should be scale.options.minBarLength
6a0de54
to
a567030
Compare
@simonbrunel thanks for the review. Rebased onto master and changes asked by were made. |
What about 0-values? This option also shows the minimum bar length for the 0 values, which I don't think is what a user usually wants... |
Good point. I'll make an adjustment. |
@adube @ahonen89 I thought it was exactly the point of this PR: allow to display a tiny bar for 0 values.
That's actually a feature request (see this #2929 comment) so I think we should apply |
@simonbrunel Oh, so I'll revert what I just did, then. Hold on... |
Same here #3915 (this comment for example). @adube can you confirm that your PR also fix #3915? |
Just checked and actually setting |
We could make minBarLength: function(context) {
// 5px bar length for non zero values, else 0
return context.dataset.data[context.dataIndex] ? 5 : 0
} Though, that would require much more effort / changes, so I would not implement it now. We can still merge and add the scriptable part in another PR (would not be a breaking change). What do you think? Any other idea? |
I don't know. It's great that this PR also solves that issue (3915). |
Actually this is a great idea, allowing a lot of flexibility. |
Thank you @adube for your contribution and @ahonen89 for your feedback. If you guys want to have a look to make it scriptable, you can refer to the bubble controller (we should first focus on this specific |
I realize that I have a (pretty old) branch with an attempt to make bar chart options scriptable. If I can find time to work on Chart.js, I will try to revive it and submit a PR as soon as I can (no promise though). |
does this work.... tried many combos and none worked.... v2.9.4 to be clear where type = time scales: { |
Hello. It's been a while since I last checked, but this PR was about the introduction of Are you sure your comment is about what was introduced in this PR? Thanks and happy coding! |
@adube yes i was trying minBarLength and didn't do anything..., i know the above doesnt have it, that was simply to show that im using time for my scale. could you point me to a working example using minBarLength, im using the plugin and that works... i figure it must just not function on the type "time" not that i tested others... was just very confusing, as it should be simple. |
+1 to @tomzag 's comment on stacked bar charts |
@tomzag @claireebilski any workaround on the stacked overlap problem ? should we open an new issue on github ? |
This patch introduces theminSize
dataset property for bar charts.This patch introduces the
minBarLength
scale property for bar charts. The value, if set, defines the minimum size a bar should be rendered when the value would be too low for the bar to appear visible.Works for both vertical and horizontal bars.
Fixes #2959
Fixes #3915
Live example on jsbin: https://jsbin.com/rodurop/edit?html,js,output