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

Introduce the 'minBarLength' dataset property for bar scales (X or Y) #5741

Merged
merged 4 commits into from Oct 18, 2018

Conversation

adube
Copy link
Contributor

@adube adube commented Sep 20, 2018

This patch introduces the minSize 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

Edit(SB): "Fixes #xxxx" to close associated tickets

@benmccann
Copy link
Contributor

I'm wondering if this should be called minHeight instead of minSize to make it clearer which dimension it's referring to and partially to differentiate from barThickness

@adube
Copy link
Contributor Author

adube commented Sep 24, 2018

So we would have minHeight for vertical bars and minWidth for horizontal ones, is that correct?

@benmccann
Copy link
Contributor

I would lean towards calling it minHeight everywhere. Thoughts @simonbrunel @etimberg ?

@simonbrunel
Copy link
Member

simonbrunel commented Sep 24, 2018

I agree that size could be confusing with thickness but I think I prefer minSize (or another single option) over minHeight and minWidth because we don't need to change the option when switching the chart orientation. It's also less confusing, for example, if you define both in your default options (because you need to display horizontal and vertical charts), what minWidth means for a vertical bar?

Calling minHeight for both orientations is confusing IMO, don't you think?

@benmccann
Copy link
Contributor

I agree we should switch between minWidth and minHeight depending on orientation

I didn't think that using minHeight for both was confusing, but sounds like you both do. What about minLength as another alternative? (then we would have thickness and length)

@adube
Copy link
Contributor Author

adube commented Sep 24, 2018

What about minLength as another alternative? (then we would have thickness and length)

I like the minLength suggestion.

@simonbrunel
Copy link
Member

What about minBarLength, similar to maxBarThickness? Also, I'm not sure it should be set on the dataset but in the scale options to be consistent with maxBarThickness.

That being said, I think that bar specific options existing at the scale level (e.g. barPercentage, barThickness, etc.) should be deprecated and relocated under dataset because it's specific to the bar controller type. But for now I would try to keep things consistent and use scale.minBarLength until we decide to move these options.

What do you think?

@adube
Copy link
Contributor Author

adube commented Sep 25, 2018

What about minBarLength, similar to maxBarThickness? Also, I'm not sure it should be set on the dataset but in the scale options to be consistent with maxBarThickness.

That being said, I think that bar specific options existing at the scale level (e.g. barPercentage, barThickness, etc.) should be deprecated and relocated under dataset because it's specific to the bar controller type. But for now I would try to keep things consistent and use scale.minBarLength until we decide to move these options.

What do you think?

I agree.

Unless said otherwise, I'll proceed with making these adjustments and implement scale.minBarLength instead of dataset.minSize.

@adube
Copy link
Contributor Author

adube commented Sep 25, 2018

For the record, with minBarLength as scale option, this is what I ended up having to use as config:

For a vertical bar chart (on yAxes):

scales: {
	yAxes: [{
		minBarLength: 4
	}]
}

For an horizontal bar chart (on xAxes):

scales: {
	xAxes: [{
		minBarLength: 4
	}]
}

@adube
Copy link
Contributor Author

adube commented Sep 25, 2018

Patch updated. Please, let me know if I need to re-create a live example out of this.

@benmccann
Copy link
Contributor

Cool. Can you add a unit test?

@adube
Copy link
Contributor Author

adube commented Sep 25, 2018

Cool. Can you add a unit test?

Sure.

@adube
Copy link
Contributor Author

adube commented Oct 2, 2018

Please, let me know if there's anything else I could do to help pushing this new feature forward. Thanks.

benmccann
benmccann previously approved these changes Oct 4, 2018
@adube adube changed the title Introduce the 'minSize' dataset property for bar controller Introduce the 'minBarLength' dataset property for bar scales (X or Y) Oct 5, 2018
@@ -30,6 +30,8 @@ module.exports = function(Chart) {
return isHorizontal ? meta.xAxisID === me.id : meta.yAxisID === me.id;
}

me.minBarLength = opts.minBarLength;
Copy link
Member

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.

var datasets = chart.data.datasets;
var value = scale.getRightValue(datasets[datasetIndex].data[index]);
var minBarLength = scale.minBarLength;
Copy link
Member

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

@adube
Copy link
Contributor Author

adube commented Oct 17, 2018

@simonbrunel thanks for the review.

Rebased onto master and changes asked by were made.

@ahonen89
Copy link

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...

@adube
Copy link
Contributor Author

adube commented Oct 18, 2018

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.

@simonbrunel
Copy link
Member

simonbrunel commented Oct 18, 2018

@adube @ahonen89 I thought it was exactly the point of this PR: allow to display a tiny bar for 0 values.

... I don't think is what a user usually wants...

That's actually a feature request (see this #2929 comment) so I think we should apply minBarLength to all values (even 0). In case you want to use minBarLength and hide 0 values, then you should set null or undefined in your data.

@adube
Copy link
Contributor Author

adube commented Oct 18, 2018

@simonbrunel Oh, so I'll revert what I just did, then. Hold on...

@simonbrunel
Copy link
Member

Same here #3915 (this comment for example). @adube can you confirm that your PR also fix #3915?

@ahonen89
Copy link

Just checked and actually setting null or undefined for a data value removes the possibility to show the tooltip on that bar.

@simonbrunel
Copy link
Member

@ahonen89 so what would be your suggestion to implement #3915?

@simonbrunel
Copy link
Member

We could make minBarLength scriptable:

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?

@ahonen89
Copy link

@ahonen89 so what would be your suggestion to implement #3915?

I don't know. It's great that this PR also solves that issue (3915).
But maybe it can be further extended to allow more flexibility and maybe allow to specify an interval of data for which the minimum bar length will not be shown...
For example, for a chart with big values (let's say > 1 milion), 1k and 1 will have the same minimum bar length specified with the current option. Having an interval option [0, 500] that will not be taken in consideration for the minimum bar length can be an solution for some users of the chart.

@ahonen89
Copy link

We could make minBarLength scriptable:

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?

Actually this is a great idea, allowing a lot of flexibility.

@simonbrunel
Copy link
Member

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 minBarLength option, then add other ones later). If you don't need this feature right now, you can still wait until we convert all bar options to be fully scriptable.

@simonbrunel
Copy link
Member

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).

@Seabizkit
Copy link

Seabizkit commented Nov 7, 2020

does this work.... tried many combos and none worked.... v2.9.4

to be clear where type = time

scales: {
xAxes: [{
type: 'time',
time: {
unit: 'day',
}
}],
yAxes: [{
ticks: {
beginAtZero: true,
}
}]
},

@adube
Copy link
Contributor Author

adube commented Nov 9, 2020

Hello. It's been a while since I last checked, but this PR was about the introduction of minBarLength, and I'm not seeing it in your example.

Are you sure your comment is about what was introduced in this PR?

Thanks and happy coding!

@Seabizkit
Copy link

@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.

@tomzag
Copy link

tomzag commented Dec 24, 2020

There seems to be a problem with stacked bar charts.

Here for example a horizontal stacked bar chart:
Bildschirmfoto 2020-12-24 um 13 14 44

The minBarLength works but the stacked version isn't respecting it and the bars overlap.

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
@claireebilski
Copy link

+1 to @tomzag 's comment on stacked bar charts

@nabenzine
Copy link

@tomzag @claireebilski any workaround on the stacked overlap problem ? should we open an new issue 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.

[FEATURE] Show zero values in bar graph. min-height for bars in bar chart
9 participants