-
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
Make offsetGridLines
behavior consistent and add new offset
option to scales
#4545
Conversation
@nagix thanks for the incredibly detailed PR notes. I will try and look at the code soon. |
src/scales/scale.category.js
Outdated
@@ -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 |
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.
typo chaer
src/scales/scale.time.js
Outdated
@@ -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 |
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 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.
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.
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
.
src/scales/scale.time.js
Outdated
// 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; |
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.
if you set this variable outside the if
then you can use it in the if
check
docs/charts/bar.md
Outdated
@@ -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. |
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.
calcurated -> calculated
Below are the time scale charts with |
@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 |
@benmccann Thanks for reviewing this, and I understand your concern. To make it simpler, how about the following idea?
|
That sounds reasonable to me. @simonbrunel thoughts? Simon and I were talking about the one point below
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 |
Here's a sample that can be used to reproduce the bug I mentioned above when |
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 |
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
|
@benmccann The issue you reported has been fixed. Also, the last bar is plotted on a time scale, which was reported in #3297. |
@nagix can you squash and rebase? |
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 |
@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! |
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 When |
offsetGridLines
behavior consistent and add new sampleSize
optionoffsetGridLines
behavior consistent and add new ticks.offset
option
docs/axes/cartesian/README.md
Outdated
@@ -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. |
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.
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
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.
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.
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.
Thanks @nagix! There are some great changes here! |
src/controllers/controller.bar.js
Outdated
curr = scale.getPixelForValue(null, i, datasetIndex); | ||
data.push(curr); | ||
minInterval = Math.min(minInterval, curr - prev); | ||
if (ticksOffset) { |
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.
Do you need this if/else? Could you simplify by just always checking that curr > min && curr < max
?
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.
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.
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
@nagix is this PR ready to be merged? |
@simonbrunel Yes, it is ready. |
Woo! This is great! Thanks so much @nagix! Will you be submitting a PR to add |
yay! Thank you! Do you think this would warrant a new release? |
@nagix Thanks |
@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). |
@andig |
…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
@andig |
@myadav-etindia See #4706 |
My team previously extended |
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 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? |
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 |
@benmccann Thanks. So that just plain doesn't exist, huh? I guess I'll look into how to implement such a thing. |
@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.
…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.
Document says
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
isfalse
, 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 whenoffsetGridLines
isfalse
.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
isfalse
. 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 totrue
. 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 theoffsetGridLines
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.
chart.isCombo
includeOffset
argument fromgetPixelForValue()
andgetPixelForTick()
offset
option to scales to add extra space (half tick interval) at edgesoffsetGridLines
is true, grid lines move to the left by one half of the tick interval, and labels don't moveThe 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