Make `offsetGridLines` behavior consistent and add new `offset` option to scales #4545

nagix · 2017-07-21T21:07:23Z

offsetGridLines

If true, the bars for a particular data point fall between the grid lines. If false, the grid line will go right down the middle of the bars.

However, the current implementation doesn't change the positions of grid lines, and bars stay between the grid lines regardless of whether it is true or false. Instead, only the positions of labels are shifted.

When a chart has mixed data types (e.g. bar and line) and offsetGridLines is false, the grid line goes right down the middle of the bars as the document explains. This seems to me the correct behavior even for non-mixed charts when offsetGridLines is false.

Another problem is that the right most bar is completely hidden (in case of a bar chart) or bars at the both edges are hidden partially (in case of a mixed chart) when offsetGridLines is false. Calculation of the bar width is also incorrect.

In case of time scale charts, offsetGridLines has no effect, and bars are drawn as if it were set to true. It makes more sense to me that the center of a bar is positioned at the time that the data point refers as the default behavior. The behavior of the offsetGridLines option should be consistent across different scales.

The other issue is that the width of bars and padding are calculated based on the number of the ticks. If the number of the samples are fewer or more than the number of the ticks, unnecessary margin or overlaps will appear.

This PR contains the following changes to resolve the issues above.

Remove chart.isCombo
Remove the includeOffset argument from getPixelForValue() and getPixelForTick()
Add a new offset option to scales to add extra space (half tick interval) at edges
Bar controller automatically calculates the bar width to avoid overlaps
When offsetGridLines is true, grid lines move to the left by one half of the tick interval, and labels don't move
Add tests to the scales and the bar controller

The following images compare the current version (on the left) and the proposed version (on the right).

Below are charts with time scales.

Below are examples of horizontal bar charts.

Below are examples of non-linear time scales.

Fixes #2323
Fixes #2415
Fixes #3263
Fixes #3297
Fixes #3579
Fixes #3613
Fixes #3682
Fixes #3913
Fixes #4051
Fixes #4343
Fixes #4471
Fixes #4560

etimberg · 2017-07-21T22:13:18Z

@nagix thanks for the incredibly detailed PR notes. I will try and look at the code soon.

benmccann · 2017-07-22T07:28:11Z

src/scales/scale.category.js

@@ -46,6 +46,11 @@ module.exports = function(Chart) {
 			var labels = me.getLabels();
 			// If we are viewing some subset of labels, slice the original array
 			me.ticks = (me.minIndex === 0 && me.maxIndex === labels.length - 1) ? labels : labels.slice(me.minIndex, me.maxIndex + 1);
+
+			// Disable zero line if a chaer has bars and offsetGridLines is false


typo chaer

benmccann · 2017-07-22T07:33:16Z

src/scales/scale.time.js

@@ -281,6 +281,29 @@ module.exports = function(Chart) {
 				max = ticks.length ? ticks[ticks.length - 1] : max;
 			}

+			// If the chart has bars, shift min and max based on the sample size


I think you'll need a different logic here if mode: series

ticks.mode ('linear'(default)|series): series displays ticks at the same distance from each other, whatever the time value they represent, while linear displays them linearly in time: the distance between each tick represent the amount of time between their time values.

Good point! As each tick interval can have different value within the same scale, the sample size of bars can also very. So, I removed the ruler code from controller.bar.js, and made calculateBarIndexPixels() call getBarPixelsForIndex() to get the left and right edge of the bar for a specified index. I also modified the calculation logic in scale.time.js to support ticks.mode and ticks.source.

benmccann · 2017-07-22T07:36:03Z

src/scales/scale.time.js

+			// If the chart has bars, shift min and max based on the sample size
+			if (me.options.gridLines.offsetGridLines || me.chart.hasBar) {
+				var sampleSizeMs = moment.duration(me.options.sampleSize || 0).asMilliseconds();
+				var offsetGridLines = me.options.gridLines.offsetGridLines;


if you set this variable outside the if then you can use it in the if check

benmccann · 2017-07-22T07:38:11Z

docs/charts/bar.md

@@ -116,6 +117,13 @@ options = {
 }
 ```

+### sampleSize
+If specified, the width for each sample will be calcurated based on this value. If not specified, it will be the available width (the space between the gridlines). This only works if `barThickness` is not set.


calcurated -> calculated

nagix · 2017-07-23T16:46:53Z

Below are the time scale charts with gridLines.offsetGridLines = true, tick.mode = linear | series and tick.source = auto | labels. It looks ok now. There are overlaps when gridLines.offsetGridLines is false, but that problem cannot be solved when tick time and sample time are not aligned, and the fixed sample size (sampleSize option) should be used in that case.

benmccann · 2017-07-24T06:20:10Z

I think there's a bug here with mode: series and offsetGridLines: false. See the screenshot below. It causes the bars to be different widths.

benmccann · 2017-07-24T09:22:01Z

@nagix I want to thank you for the truly awesome description and your attention to testing. We really appreciate all the time you've put into fixing this issue. This is a very important issue with a lot of benefit given how many times people have reported it

I think there's one main issue, which is that it's making many other classes aware of the bar chart when they shouldn't need to know about the bar chart. It would be preferable to keep the fix contained within the bar chart as much as possible. If the bar chart looks at the ticks of the axis hopefully it should be possible to calculate the bounds of the bar

nagix · 2017-07-24T10:14:07Z

@benmccann Thanks for reviewing this, and I understand your concern. To make it simpler, how about the following idea?

Don't use sampleSize option
The default bar size is the size of the tick interval that the data sample is on
- In case of a category scale, it will be a fixed size. In case of a time scale, it will depend on the tick configuration
When detecting overlap with the next bars, recalculate the size individually so that they fit each other

benmccann · 2017-07-24T15:22:46Z

That sounds reasonable to me. @simonbrunel thoughts?

Simon and I were talking about the one point below

The default bar size is the size of the tick interval that the data

I think we came to the conclusion that it might be nice to have an option that switches this between two modes. One is the mode you mentioned. The other is a mode where you force the bars to be equally sized (the size of the closest two ticks). Here's a use case for what I'm talking about: imagine you're plotting a daily stock with a bar chart. You set mode:linear. I think you would want five equally sized bars with a space for the weekend followed by five equally sized bars for the next week. In this case it would be strange if the bars for Monday and Friday were much bigger than the bars for Tue, Wed, Thurs

benmccann · 2017-07-25T04:52:33Z

Here's a sample that can be used to reproduce the bug I mentioned above when mode: series and offsetGridLines: false: https://raw.githubusercontent.com/benmccann/Chart.js/offsetGridlineExample/samples/scales/time/financial.html

benmccann · 2017-07-26T03:35:07Z

Also, I haven't tested with your PR, but currently on master, if you try to plot a bar on a time scale, there's one point that doesn't get plotted (I'm not sure if it's the first or the last). That might be something else we want to test for

nagix · 2017-07-28T03:58:37Z

I have changed the code so that the bar controller and the scales don't depend on each other as much as possible. Now bar controller calculates the bar width based on the return value of the getPixelForValue function.

Remove sampleSize option
Calculate the grid line offset based on the minimum interval between samples
Add code to support ticks.mode and ticks.source option in the time scale
Modify the interpolation table to support lookup for tick indexes
Adjust the range of scales so that all bars can be displayed

nagix · 2017-07-28T04:01:54Z

@benmccann The issue you reported has been fixed. Also, the last bar is plotted on a time scale, which was reported in #3297.

simonbrunel · 2017-07-28T05:24:07Z

@nagix can you squash and rebase?

simonbrunel · 2017-07-28T06:35:29Z

Thanks @nagix for this awesome work. I think there is still one (implementation) issue, can you post a screenshot of bars on a time scale with mode: 'linear', source: 'labels', with labels at irregular times:

benmccann · 2017-07-31T04:57:44Z

@nagix will you be able to squash and rebase to get the latest commits from master to stop showing up in your PR? i'm very excited for this improvement to make its way in!

nagix · 2017-08-05T19:32:19Z

Sorry for the late response. I did squash and rebase to the latest master.

I have also added several tests to the time scale, category scale and bar controller to cover the ticks.offset, barThickness and maxBarThickness options.

When barThickness is not specified, the bar width will be sized based on the minimum interval between bars in the chart to avoid overlaps, and all bars will have the same width. When 'max' is set to barThickness, the bar width will be maximized individually, and each bar can have a different width.

benmccann · 2017-08-05T21:03:36Z

docs/axes/cartesian/README.md

@@ -31,6 +31,7 @@ The following options are common to all cartesian axes but do not apply to other
 | `maxRotation` | `Number` | `90` | Maximum rotation for tick labels when rotating to condense labels. Note: Rotation doesn't occur until necessary. *Note: Only applicable to horizontal scales.*
 | `minRotation` | `Number` | `0` | Minimum rotation for tick labels. *Note: Only applicable to horizontal scales.*
 | `mirror` | `Boolean` | `false` | Flips tick labels around axis, displaying the labels inside the chart instead of outside. *Note: Only applicable to vertical scales.*
+| `offset` | `Boolean` | `false` | If true, extra space is added to the both edges and the axis is scaled to fit into the chart area. This is set to `true` in the bar chart by default.


Sounds quite similar to the scale.bounds feature that was implemented in #4556 (docs are being added in #4592). Is it possible to use that option? I'm not quite sure if this is doing the same thing or not

The scale.bounds feature is only in the time scale right now, but there's a desire to add it to the other scales as well, so that might be better than creating a new option if possible

Here's a couple screenshots to show the scale.bounds option

scale.bounds: 'data'

scale.bounds: 'ticks'

I think bounds: 'ticks' is similar to offset: true?

When scale.bounds: 'ticks' but the first and last data points are on the ticks, the scale won't add extra spaces at edges (see 5 below). However, offset: true will always add them to make sure that the whole bar rectangles are displayed (see 7 below).

Internally, offset: true doesn't change the time/pos lookup table. It just add one half size of the next tick intervals in getPixelForValue() (or getPixelForOffset() in case of a time scale), and this can be easily applied to other scales such as a category scale.

benmccann · 2017-08-05T21:24:24Z

Thanks @nagix! There are some great changes here!

benmccann · 2017-08-05T22:02:03Z

src/controllers/controller.bar.js

+				curr = scale.getPixelForValue(null, i, datasetIndex);
+				data.push(curr);
+				minInterval = Math.min(minInterval, curr - prev);
+				if (ticksOffset) {


Do you need this if/else? Could you simplify by just always checking that curr > min && curr < max?

We need to handle an edge case like that a dataset contains only one data point that is on the edge of a scale, the scale has only one tick and ticks.offset: false. In this case, we cannot calculate the bar width based on a tick interval or the distance from the nearest edge (because it is on the edge), so it uses the distance from the farthest edge.

andig · 2017-08-09T19:02:23Z

Checking in here thanks to @etimberg. Amazing PR! Could you kindly share any fiddle that shows a combined bar/line chart, preferably with lines using x/y notation instead of labels?

… scales - Remove `chart.isCombo` - Remove the includeOffset argument from `getPixelForValue()` and `getPixelForTick()` - Add a new `offset` option to scales to add extra space at edges - Bar controller automatically calculates the bar width to avoid overlaps - When `offsetGridLines` is true, grid lines move to the left by one half of the tick interval, and labels don't move - Add tests to the scales and the bar controller

simonbrunel · 2017-08-13T17:17:39Z

@nagix is this PR ready to be merged?

nagix · 2017-08-13T20:05:33Z

@simonbrunel Yes, it is ready.

benmccann · 2017-08-14T15:30:06Z

Woo! This is great! Thanks so much @nagix!

Will you be submitting a PR to add barThickness: 'min' next?

maxwell · 2017-08-14T21:25:40Z

yay! Thank you! Do you think this would warrant a new release?

myadav-etindia · 2017-08-18T07:06:23Z

@nagix
Just wanted to confirm if all the fixes done as part of this PR are available in chart.js release. We are using version 2.6.0 and that does not contain these fixes.
By when these fixes will be available ?

Thanks

andig · 2017-08-18T07:36:36Z

@myadav-etindia this PR is in the master branch and will probably released with 2.7.0. See http://www.chartjs.org/docs/master/developers/ for how to get access to the development releases (= master branch).

myadav-etindia · 2017-08-18T09:46:05Z

@andig
Thanks. Got the latest.

…hartjs#4545) Add a new `offset` option to scales to add extra space at edges and remove the `includeOffset` argument from `getPixelForValue()` and `getPixelForTick()`. The bar controller now automatically calculates the bar width to avoid overlaps. When `offsetGridLines` is true, grid lines move to the left by one half of the tick interval, and labels don't move.

Thanks nagix chartjs/Chart.js#4545

myadav-etindia · 2017-09-01T04:45:43Z

@andig
Tentatively by when would the chart.js 2.7.0 release be available ?

andig · 2017-09-01T08:53:42Z

@myadav-etindia I wouldn't know really- it's a question for the maintainers. My feeling is though that there is still a lot of movement that would not warrant a realease in the near future. You could still use a snapshot of the current master right now.

See #4706

buddyp450 · 2017-10-16T14:50:38Z

My team previously extended elements.rectangle because the existing Rectangle element assumed that the 'x' was the central point whose x₁ and x₂ were calculated dividing the width by 2. Our extended element assumes that x is the start and x + width = x₂. I'll need to check this out to see if this is even needed anymore but I'm assuming it's not which would be a huge boon for us.

Offlein · 2017-10-25T18:38:33Z

I was really pleased with the work done here, but one thing I'm confused about is the intention of how this handles time series line charts.

I have daily data that I'd like to present with labels grouped and labeled by month. That is, the gridLine should appear at the first of the month, but I'd expect the tick label (say "Sep" or "Jan") should appear centered between the two. It seems like with offsetGridLines: true, it appears visually the way one would expect, but the gridlines correspond to the halfway point in the month.

This fiddle illustrates what I mean: https://jsfiddle.net/y152r1nh/4/

Hovering over the point on the line before the label "Jan" gives me dates for "December 15". Am I misunderstanding how to use this feature, or is that an oversight?

benmccann · 2017-10-27T15:09:31Z

This feature is moving the gridlines for bar charts. I.e. if you're drawing a bar chart and don't want the grid line through the middle of the bar then move the gridlines over. What's you're asking for is a feature probably more like offsetLabels: true

Offlein · 2017-10-27T15:18:54Z

@benmccann Thanks. So that just plain doesn't exist, huh? I guess I'll look into how to implement such a thing.

flausinoth · 2017-11-22T13:32:48Z

@nagix What if you have an interval of days? for example: from day 20 to day 23. When setting the values, the bar graph distorts.

…hartjs#4545) Add a new `offset` option to scales to add extra space at edges and remove the `includeOffset` argument from `getPixelForValue()` and `getPixelForTick()`. The bar controller now automatically calculates the bar width to avoid overlaps. When `offsetGridLines` is true, grid lines move to the left by one half of the tick interval, and labels don't move.

benmccann mentioned this pull request Jul 22, 2017

offsetGridLines not working for bar chart #3550

Closed

benmccann reviewed Jul 22, 2017

View reviewed changes

nagix force-pushed the issue-2323 branch from 6b90c96 to 524629d Compare July 28, 2017 05:45

nagix force-pushed the issue-2323 branch from 11281b7 to cffd2bc Compare August 5, 2017 19:15

nagix changed the title ~~Make offsetGridLines behavior consistent and add new sampleSize option~~ Make offsetGridLines behavior consistent and add new ticks.offset option Aug 5, 2017

benmccann reviewed Aug 5, 2017

View reviewed changes

simonbrunel mentioned this pull request Aug 7, 2017

Time scale bar charts overlap #2323

Closed

etimberg mentioned this pull request Aug 9, 2017

Feature: support bar charts over times scales #4636

Closed

andig mentioned this pull request Aug 9, 2017

Howto Combined line/bar chart with x/y lines #4498

Closed

simonbrunel merged commit 7dc71d0 into chartjs:master Aug 14, 2017

nagix deleted the issue-2323 branch August 15, 2017 08:40

This was referenced Aug 17, 2017

[BUG] Wrong width and position of non-equidistant bars with {x, y} data points #4666

Closed

[BUG] Fill didn't start from zeroLine in mixed lines/bar chart #4668

Closed

andig mentioned this pull request Aug 17, 2017

Bar size limits #4561

Closed

brewingcode added a commit to brewingcode/trip-track that referenced this pull request Aug 23, 2017

Fix x-axis label spacing

64669df

Thanks nagix chartjs/Chart.js#4545

Zamaroth mentioned this pull request Sep 28, 2017

Move drawing of gridLines into separate plugin and add border functionality #4117

Closed

simonbrunel mentioned this pull request Jan 22, 2018

[BUG] bar charts scales are inconsistent when type: 'time' and unit: 'week' OR 'month' #3747

Closed

simonbrunel mentioned this pull request Feb 7, 2018

Line chart with only one value #5245

Closed

nagix mentioned this pull request Jun 26, 2018

Update the descriptions of barThickness, offsetGridLines and offset #5600

Merged

nagix mentioned this pull request Aug 13, 2018

Fix offsetGridLine behavior with a single data point #5609

Merged

nagix mentioned this pull request May 22, 2019

Overlapping Bar Chart with Smaller Inner Bar & Thicker Outer Bar using 2.7.1 #5224

Closed

thisisiris mentioned this pull request Sep 28, 2021

[URGENT] Trying to replicate results on 3.4.1 #9689

Closed

Make offsetGridLines behavior consistent and add new offset option to scales #4545

Make offsetGridLines behavior consistent and add new offset option to scales #4545

Conversation

nagix commented Jul 21, 2017 • edited

offsetGridLines

etimberg commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagix commented Jul 23, 2017

benmccann commented Jul 24, 2017 • edited

benmccann commented Jul 24, 2017 • edited

nagix commented Jul 24, 2017

benmccann commented Jul 24, 2017

benmccann commented Jul 25, 2017

benmccann commented Jul 26, 2017

nagix commented Jul 28, 2017

nagix commented Jul 28, 2017

simonbrunel commented Jul 28, 2017

simonbrunel commented Jul 28, 2017

benmccann commented Jul 31, 2017

nagix commented Aug 5, 2017

benmccann Aug 5, 2017 • edited

Choose a reason for hiding this comment

benmccann Aug 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andig commented Aug 9, 2017

simonbrunel commented Aug 13, 2017

nagix commented Aug 13, 2017

benmccann commented Aug 14, 2017

maxwell commented Aug 14, 2017

myadav-etindia commented Aug 18, 2017

andig commented Aug 18, 2017

myadav-etindia commented Aug 18, 2017

myadav-etindia commented Sep 1, 2017

andig commented Sep 1, 2017 • edited

buddyp450 commented Oct 16, 2017

Offlein commented Oct 25, 2017

benmccann commented Oct 27, 2017

Offlein commented Oct 27, 2017

flausinoth commented Nov 22, 2017

Make `offsetGridLines` behavior consistent and add new `offset` option to scales #4545

Make `offsetGridLines` behavior consistent and add new `offset` option to scales #4545

nagix commented Jul 21, 2017 •

edited

benmccann commented Jul 24, 2017 •

edited

benmccann commented Jul 24, 2017 •

edited

benmccann Aug 5, 2017 •

edited

benmccann Aug 5, 2017 •

edited

andig commented Sep 1, 2017 •

edited