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

[BUG] Chart.js grid lines and axes not working in v2.7.1 as they did in v2.4.0 #5017

Closed
seancclark opened this issue Dec 2, 2017 · 11 comments · Fixed by #8781
Closed

[BUG] Chart.js grid lines and axes not working in v2.7.1 as they did in v2.4.0 #5017

seancclark opened this issue Dec 2, 2017 · 11 comments · Fixed by #8781

Comments

@seancclark
Copy link

seancclark commented Dec 2, 2017

Appear to have found a bug (or something has changed between version 2.4.0 and v2.7.1).

Working example with v2.4.0: https://jsfiddle.net/juk94xpo/
However chart doesn't look as it should if you upgrade to v2.7.1: https://jsfiddle.net/1sqkztto/

It seems that v2.7.1 does not draw tick marks in the correct position and axes disappear. This worked correctly in v2.4.0.

@etimberg
Copy link
Member

etimberg commented Dec 2, 2017

Thanks for reporting this @seancclark. I looked at both fiddles and the canvas is the same size in both.

The issue with the grid lines looks like it changed in v2.5.0. I think the change boils down to starting with v2.5, the step is incremented from the axis minimum value while in older versions it seems to have been centered around 0.

I think a different, easier to use, mechanism needs to be created for specifying tick marks at certain values.

@simonbrunel @benmccann how would you feel some of the following options:

  • An optional setting that is an array of tick values, and if specified, overrides min, max and stepSize
  • Allowing a function to be specified for the ticks (using the scriptable options perhaps?)

@simonbrunel
Copy link
Member

An optional setting that is an array of tick values, and if specified, overrides min, max and stepSize

Isn't it the same as the scale labels options (#4506), but applied to all scales?

Allowing a function to be specified for the ticks

What would be inputs/outputs of this function? Scriptable options are mapped to the dataset/data, doesn't really make sense for ticks.

@etimberg
Copy link
Member

etimberg commented Dec 2, 2017

Yup, like labels but in the scale options.

options {
  scales: {
    yAxes: [{
      ticks: {
        values: [0, 19, 43, 50] // implies min == 0, max == 50
      }
    }]
  }
}

The function idea would take the data range and return the ticks. Ticks outside the range would be ignored.

options {
  scales: {
    yAxes: [{
      ticks: {
        generator: function(range)  {
          return [0, 10, 20];
        }
      }
    }]
  }
}

@simonbrunel
Copy link
Member

#4506 is about labels in the scale options, but only for the category scale IIRC, so would generalize this concept to all scale for consistency.

@etimberg
Copy link
Member

etimberg commented Dec 3, 2017

Yup! That would fix 2 issues at once 😄

@seancclark seancclark changed the title [BUG] Chart.js not honouring canvas size in v2.7.1 as it did in v2.4.0 [BUG] Chart.js grid lines and axes not working in v2.7.1 as they did in v2.4.0 Dec 3, 2017
@jcopperfield
Copy link
Contributor

@etimberg I think having different configuration options tick.values and tick.generator is overkill. I think the ChartJS code should be smart enough to check the type of the option.
e.g.

if (typeof values === 'function') {
    values = values(generationOptions, dataRange);
}
if (!values) {
    // generate default
}

@simonbrunel
Copy link
Member

Agree with @jcopperfield, that's what we do with scriptable options. Though, I would keep the scale.labels options introduced in #4506 (for consistency) and generalize this feature to all scales (should be relatively easy at the core.scale.js level).

options {
  scales: {
    yAxes: [{
      labels: [0,10,20],
      // or
      labels: function(context) {
         // context: {chart, range?, ...)
         return [...];
      }
    }]
  }
}

Actually, you might be right @etimberg, that could be a scriptable option with a scale specific context.

@jcopperfield
Copy link
Contributor

@simonbrunel didn't know there scriptable options were already implemented. However wouldn't it be better to call the scriptable option function using value.apply(context, arguments_array);?

@simonbrunel
Copy link
Member

Why would it be better?

@jcopperfield
Copy link
Contributor

So that someone trying to override the function would get the same arguments as the original function. For instance for the generator function(generationOptions, dataRange) and for a formatter function(tickValue, index, ticks) instead of having a context object.

@simonbrunel
Copy link
Member

That would make the whole scriptable concept hard to understand and maintain because not consistent. I doubt there is any advantage to map the generators/formatters signatures with our options. The context object should be simple and the same for all options under scale, making also easier to share these functions between options: the context is not related to the option itself. Also keeping the function signature simple (only a single argument) helps to prevent breaking changes when introducing/deprecating properties since we don't care about the arguments order.

In the case you want to write a custom generator/formatter, then it's easier to override/augment these methods directly (there is a ticket to make them public). If you want a very custom labels array, based on the chart data for example, then create a scriptable option and do whatever you want with context.chart.data or context.scale for example. You have almost access to everything and I don't think we should expose more specific properties to this context (I would not expose any range).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants