carbon-design-system / carbon-charts

:bar_chart: :chart_with_upwards_trend:⠀Robust dataviz framework implemented using D3 & typescript
https://charts.carbondesignsystem.com
Apache License 2.0
904 stars 183 forks source link

[Bug]: Bar width needs to be scaled by the number of time periods #1554

Open mthaak opened 1 year ago

mthaak commented 1 year ago

Name

Martin

Are you an IBM employee?

~- [ ] Yes~

What happened?

Not sure if this is a bug or just something that can be improved...

The problem is that for bar charts that use a time scale the bar width calculation doesn't really make sense if the bars are not distributed evenly:

return Math.min(options.bars.maxWidth, (chartWidth * spacingFactor) / numberOfDatapoints);

At: https://github.com/carbon-design-system/carbon-charts/blob/55edb10abf9b2d5b7ffca8f78c019984ae03ac75/packages/core/src/components/graphs/bar.ts#L27

In the calculation, the chartWidth * spacingFactor is divided by the number of data points to determine how much space each bar needs. However, the number of data points cannot be used as a reliable divisor because the bars may not be evenly distributed across the time domain.

See this example: image

The bar width is scaled with the assumption that the bars are distributed evenly. But because the bars are clustered at the start and end of the time domain, they overlap.

I'm making the assumption that the time scale will usually be applied for data that is mapped to evenly spaced time periods (e.g. per second, minute, hour, day, month, year, etc.). Given that, it would be better if the bar width would scale with the number of time periods between the min and max date on the domain. The time period size could be an option or automatically determined from the data (see GCD).

So in the example above the number of time periods is 12, while the number of data points is 5. If the bars were scaled by the number of time periods they would be thinner and fit nicely in the chart area without overlap (though maybe there needs to be a margin to account for the space around the edges).

This is my desired bar behaviour: image

Hope this makes sense!

Version

@carbon/charts@0.57.0

Data & options used

No response

Relevant log output

No response

Codesandbox example

https://codesandbox.io/s/epic-bassi-7ml64n?file=/src/index.js

What priority level would this be in your opinion?

P0

theiliad commented 1 year ago

Hi, looking at the codesandbox example I'm not able to see this issue, would you be able to pls reproduce the bug in codesandbox or stackblitz? that way we can work more easily to debug the issue

mthaak commented 1 year ago

@theiliad I might have copied the wrong sandbox link so I've created a new one to explain the current behaviour (top chart) and desired behaviour (bottom chart)

https://codesandbox.io/s/epic-bassi-7ml64n?file=/src/index.js