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

Take controller default options into account to fix mixed charts #5997

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jan 19, 2019

This fixes mixed line and scatter charts. Issue described in #4587

Closes #5151

@benmccann
Copy link
Contributor Author

benmccann commented Jan 19, 2019

I just realized a potential issue with the implementation proposed in this PR.

What it proposes to do is merge options in the order of:

  • Global defaults
  • Chart defaults
  • Controller defaults
  • User supplied options

Unfortunately, I just realized this might break in the case that the user has manually specified an option that matches the default value. In that case we might want to keep the user supplied value, but there's no way to differentiate what's user-supplied vs default. The user may call chart.options.foo = 'bar'; chart.update(); and if foo: 'bar' was a default value there's no way to know that the user is setting it manually vs it being a default value

@benmccann
Copy link
Contributor Author

benmccann commented Jan 20, 2019

Another option which may work better would be to resolve option in a different order:

  • Global defaults
  • Chart defaults
  • Chart options
  • Dataset defaults (only applied if type set on dataset, which is only suggested in docs and samples for mixed charts. Today these are not taken into account at all, which means most mixed charts don't work)
  • Dataset options

This would be far easier to implement and wouldn't have any edge cases. In terms of usage:

chart = new Chart(ctx, {
  type: 'scatter', // lines won't be shown for this chart
  data: {
    datasets: [
        { ... }
    ]
  }
});

chart = new Chart(ctx, {
  type: 'scatter',
  showLines: true, // lines will be shown for this chart because this will take precedence
  data: {
    datasets: [
        { ... }
    ]
  }
});

chart = new Chart(ctx, {
  type: 'line',
  showLines: false, // only affects datasets[2]
  data: {
    datasets: [
        { type: 'line', ... }, // lines will be shown for this dataset
        { type: 'scatter', ... }, // lines won't be shown for this dataset
        { ... }
    ]
  }
});

// shows line for datasets[2] which uses chart-level type
chart.options.showLines = true;
chart.update();

// this would make the scatter dataset show lines
chart.data.datasets[1].showLine = true;
chart.update();

@kurkle
Copy link
Member

kurkle commented Jan 20, 2019

I'm not too familiar with this part yet, but I think that user provided options should be kept separately and applied after merging defaults. Also I'm not so sure if merging should be done at all, since if makes difficult to detect and properly apply changes after initialization.

So I wonder if all options could be resolved where used? It would leave the resolution path open. This might not make any sense 😏

@benmccann
Copy link
Contributor Author

I think that user provided options should be kept separately

I agree. Unfortunately that's not possible until v3. chart.options contains the merged defaults and user options and we can't change that without breaking backwards compatibility. The user can both read and edit chart.options today and we can't change what's there or break the ability to do that

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

Successfully merging this pull request may close these issues.

2 participants