-
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 legend appearance consistent with chart elements #5621
Conversation
To me this seems ready to merge, @etimberg, @simonbrunel ? |
Recently scriptable options have been added, so I will review the code. |
490ac6d
to
0d3a907
Compare
Added support for scriptable options and |
@nagix I think there is a deeper issue with the current implementation (master) because the legend needs to know about the dataset options logic, which is specific to the controller. I also don't think controllers should be responsible to override Thinking out loud: what about introducing a new dataset API to generate a "style", so dataset controllers would be able to return a set of predefined style properties that should be used to represent the dataset. For example (line controller): // Part of the line dataset controller
// @params {string} type - "primary" / "secondary"
// @return {IStyleInterface}
getStyle: function(type) {
var options = type === "primary"
? this._resolveLineOptions(...)
: this._resolvePointOptions(...);
return {
backgroundColor: options.backgroundColor,
borderColor: options.borderColor,
// ...
};
} For backward compatibility, we will need a base implementation at the DatasetController level. Then the legend default generateLabels: function(chart) {
var options = chart.options.legend || {};
var secondary = options.labels && legendOpts.labels.usePointStyle;
var type = secondary ? 'secondary' : 'primary';
return helpers.isArray(data.datasets) ? data.datasets.map(function(dataset, i) {
var meta = chart.getDatasetMeta(i);
var controller = meta.controller;
var style = controller.getStyle(type);
return {
text: dataset.label,
fillStyle: style.backgroundColor,
// ...
};
} : [];
} We could use What do you think about this approach? |
@simonbrunel I like this approach. |
@nagix I forgot about the per-element legend labels, so that means we need to support accessing style for the dataset or for a specific data. Maybe I'm not sure we can remove the overrides in the doughnut and polar controllers yet. Maybe we could add a new legend options to switch between per-dataset / per-data legend items (e.g. |
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.
Just some minimization comments
Problems
Legend appearance is not consistent with chart elements. For example,
usePointStyle
istrue
(legend.usePointStyle uses borderColor to fill everything #5375)usePointStyle
istrue
andpointRotation
is non-zero.fill
isfalse
.See https://jsfiddle.net/nagix/5ztb6hLc
Solutions
usePointStyle
istrue
.usePointStyle
istrue
or the dataset type is not'line'
or'radar'
.usePointStyle
istrue
.fill
isfalse
.See https://jsfiddle.net/nagix/negp2rLh
In addition to above, the existing tests are fixed and more tests are added.
Fixes #5375
Fixes #6135
Closes #2281