Fix for issues #4897, #4907

[jcopperfield]

Issues

• stacked group is not rendered correctly when stack group is not adjacent #4897: Non adjacent stacked group is not rendered correctly.
• Bar chart for only dataset.data value #4907: Bar chart for only dataset.data value.

Problems
#4897: Only the dataset index was used for indexing the stack.

#4907: Bar stretched over multiple labels for single value dataset

[etimberg]

I'm curious, which part of the issue does this fix?

[jcopperfield]

It fixes: #4907: Bar stretched over multiple labels for single value dataset.

This code is part of a special case for length === 1, in the other cases where index === 0 or index === length - 1, which is always the case, both sample sizes are set to the same value.

[etimberg]

This looks good for the stacks. The part I'm not sure about is the leftSampleSize bit but @benmccann and @simonbrunel know more about that than I do

[benmccann]

The bar implementation introduced in 2.7 doesn't make sense to me. It lets all the bars be different widths and off-center. If I type "bar chart" into Google Images then every single result has all the bars being the same width. I've never seen a compelling use case for having every bar be a different width and I'm not aware even of a user who has requested it.

The current code has a number of bugs such as #4907 and #4825. I would prefer that we invest our time in making our bar charts behave the way users expect which is to have all the bars are be the same width and centered. That's exactly what @jcopperfield is trying to do here with rightSampleSize = leftSampleSize which is to make the right half of the bar the same size as the left half of the bar. I don't think that the fix here is even close to the current intended functioning of the bar chart. But I also think the suggested fix makes way more sense than the current code (with the caveat that we should always center bars and not just when there's a single bar).

@etimberg I would really love to get your opinion on how we should approach this issue. My opinion is that our users expect bars to always be the same width. I've never seen bar charts outside of Chart.js behave the way ours does. Because the code introduce in 2.7 is so different we're going to be consistently breaking user expectations.

@jcopperfield Thanks for these fixes! I think it'd probably be better practice to have a PR address only a single bug and so we should probably split this into two PRs. It becomes harder to review and discuss when we start trying to solve multiple issues in a single PR. I'd love to just merge the fix for the stacks part and review and discuss the leftSampleSize portion on a separate PR because I think it may take quite a bit of discussion to get consensus on that part and I don't want to hold up the other half of your fix.

[jcopperfield]

Opened new PR #4937 for just issue #4897: Non adjacent stacked group is not rendered correctly.

[etimberg]

@benmccann I agree that the bars should be evenly sized and centered around the point. Some corner cases I can think of:

• bar at the edge of the graph (should it be half width or should we increase the axis size)
• bars very close together such that one side is very small

Maybe the best logic for this in our code would be to set leftSampleSize and rightSampleSize to the minimum of the two.

[benmccann]

I'm going to close this PR in favor of #4937

[simonbrunel]

I (obviously) agree that users should be able to implement this (common) case when they provide points at different distances (data: [{x,y|t}, ...]) however the current implementation doesn't need to be rewritten but instead need to be completed with an additional option (which maybe should be the default) as we discussed with @nagix.

@benmccann, the current implementation makes sense since it's the most efficient and natural way to size bar when providing data this way. It would require more arbitrary logic to determine what is the expected size and more computation which is absolutely useless when providing evenly spaced (labels+) data, which I guess is the way data are provided for the charts you seen on Google Image.

Bar with different sizes is not common for sure but it's not a reason to remove this feature (e.g. searching bar charts width). So please, don't break the current implementation because you think it's a useless feature but let's handle all cases the simplest and most optimal way.

our code would be to set leftSampleSize and rightSampleSize to the minimum of the two

That's arbitrary and would definitely break the case where we want different bar sizes. What we would need I think is a way to specify the category size relative to other values (e.g. use the thinnest category size). I don't know yet what should be this option (new or existing), but could be for instance: barThickness: null|{number}|'min'|'max'|'50%'... (or categoryThickness), with min as default meaning: 'use the thinnest category size.

[benmccann]

e.g. searching bar charts width

Will the current code allow you to create such a chart? The ones I see returned by your search are a type of chart called Marimekko chart, Mekko chart, or cascade chart. In these cases you need to specify a width, but I'm not sure that's how our code interprets the x-axis value passed in? It seems more that our chart tries to make the x-value lie somewhere within the bar, but then we calculate the left and right bounds somewhat arbitrarily ourselves. I'd be much more open to the current behavior if we fixed the way it behaved to draw Mekko charts. I think it would also simplify the code quite a bit. Here's a good description of such a cascade chart.

It would require more arbitrary logic to determine what is the expected size and more computation
which is absolutely useless when providing evenly spaced (labels+) data

I don't think it's arbitrary to say that we want bars to be equally sized nor do I think it's a correct assumption that people only want evenly spaced bars with providing evenly spaced (labels+) data. See #4983 as an example

[simonbrunel]

Will the current code allow you to create such a chart?

If we add support for controlling category sizes (in the category scale), then yes, the current behavior will work as-is and generates these kind of charts.

We both (actually all) agree that the default behavior should be changed for something much more common (equally sized bars). I really don't want to debate on what users want or not because I deeply think it's not to us to decide that by forcing a unique design (equally sized bars), so instead to totally remove the current behavior, I suggest to implement it behind an option.

I'm currently working on a fix that will add support for:

• barTickness: {number}: already supported, size of bar in pixels
• barTickness: 'min' (default)': bar size is minimized based on the smallest tick interval
• barTickness: 'flex': bar size is computed based on surrounding ticks in a way that each bar fits naturally side by side and prevents gaps is bar percentages are all 1.0 (current behavior).

I think it makes sense to re-use barTickness since barTickness: 42 should override the min or flex behavior.

@etimberg @benmccann thoughts?

[etimberg]

I like the idea of different barThickness: 'min' sounds like a good default. Though it may lead to small bars because of how the left and right edges work unless we add keep the logic that expands the scale appropriately.

[simonbrunel]

If scale.offset: false (trailing and leading bars are at the scale extremities, so cut by half), we will double their size to compute the minimum bar thickness. In all cases / implementations, we will need to determine the smallest sample size in order to keep all bar equally sized, so there will be cases where all bars are very thin (see screenshot). I can't think of another way to represent equally sized bars centered on their associated tick.

Does that make sense?

[etimberg]

Yup, makes sense. I think if we put some stuff in the docs users won’t be surprised if they get all small bars

[benmccann]

+1 both to having a default of barThickness: 'min' and counting bars at the edge as only half a bar (when scale.offset: false)

[lk-geimfari]

Any results here? Can we show all stacks now?