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

afterBuildTicks not working in 2.7.0 #4749

Closed
asfktz opened this issue Sep 12, 2017 · 6 comments
Closed

afterBuildTicks not working in 2.7.0 #4749

asfktz opened this issue Sep 12, 2017 · 6 comments
Milestone

Comments

@asfktz
Copy link

asfktz commented Sep 12, 2017

Seems like afterBuildTicks stopped working in 2.7.0

before (2.6.0)
screen shot 2017-09-12 at 15 43 16

after (2.7.0)
screen shot 2017-09-12 at 15 43 57

I used it like this:

{
  // ...
  scales: {
    // ...
    time: {
      unit: "hour",
      displayFormats: { hour: "MMM" }
    },
    afterBuildTicks: function(axis) {
      axis.ticks = _(data)
        .map("x")
        .map(function toTimestamp(dateString) {
          return _.toNumber(new Date(dateString));
        })
        .uniqBy(function(timestamp) {
          return moment(timestamp).startOf('month').format();
        })
        .value();
    }
  }
}

I also made a simple example to demonstrate it:
https://jsbin.com/titegozipu/edit?html,js,output

Try change the version of chart.js in the html

@simonbrunel
Copy link
Member

@asfktz Thank you for this detailed bug report and really sorry for the inconvenience. afterBuildTicks still work but (the supposed to be private) axis.ticks have been deprecated/removed internally because its format has changed (String to Object) :\ Changing its value in afterBuildTicks doesn't have anymore effect.

I didn't realize that these callbacks was public, I thought it was used only internally or when sub-classing a scale class. 2.7.0 (not intentionally) breaks backward compatibility when scale.ticks is accessed from these callbacks but also there is no way to alter the ticks since it's assigned at the end.

I knew it was a sensible change, maybe we should have dropped the "major" concept instead?! Not sure what the best way to fix these cases now? @etimberg any thoughts?

@asfktz FWIW and until we figure out a fix, for the first axis (with days), I think you can use the new scale.ticks.source: 'data' option. For the "month" scale, it will not work because of duplicate entries. A solution would be to generalize the scale.labels option (which should be relatively easy), then you could use scale.ticks.source: 'labels'.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Sep 12, 2017
@etimberg
Copy link
Member

Hmmm this is hard. could we provide a property for scale.ticks that reads into the object version and gets the values.

@asfktz
Copy link
Author

asfktz commented Sep 13, 2017

Thanks for the quick response, @simonbrunel.

I didn't realize that these callbacks was public, I thought it was used only internally or when sub-classing a scale class

Oh, I see.
From the user perspective, it looked safe to use because I read about it in the documentation:
http://www.chartjs.org/docs/latest/axes/?h=afterbuildticks

@asfktz FWIW and until we figure out a fix, for the first axis (with days), I think you can use the new scale.ticks.source: 'data' option. For the "month" scale, it will not work because of duplicate entries. A solution would be to generalize the scale.labels option (which should be relatively easy), then you could use scale.ticks.source: 'labels'.

It's OK (:
I can just stay with 2.6 for now.
The new options look cool, btw (:

I liked afterBuildTicks because it gave me a fine control over the ticks of each Axis.

Having the ability to build the ticks however I like is very flexible.
But I don't mind to do it differently.

Thanks for both of you for such a great library 🙏
Asaf

@DurgpalSingh
Copy link

DurgpalSingh commented Sep 14, 2017

What is the solution of this in 2.7.0. scale.ticks.source: 'labels' is not working. Can you provide a working fiddle

@benmccann benmccann mentioned this issue Oct 21, 2017
4 tasks
@benmccann
Copy link
Contributor

I don't think the major concept is actually even being used. If I delete the line which sets major then all the tests still pass:
https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.time.js#L399

The major variable is calculated at formatting time. The value calculated when the ticks were built is simply ignored:
https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.time.js#L658

So we should be able to change the ticks back so that they're no longer objects

@simonbrunel
Copy link
Member

I'm not sure I would go this way (reverting ticks as simple values), the issue being the general scale architecture that makes really complicated implementation changes, for example:

    function buildTicks() {
        var count = this.options.count;
        this.ticks = ... // generate ticks
    }

vs

    function buildTicks(count) {
        return ... // generate ticks
    }

The second option is much better when it comes to modify internal structure, for example changing the tick format and that's what I tried to do in #4573. I don't think we will find an easy fix for 2.7.1, so let's postpone this ticket for later.

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

No branches or pull requests

5 participants