-
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
Allow updating dataset types #4586
Conversation
This would allow me to remove the hack in #4554 |
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.
It's probably a good idea to include a test for this.
Logic seems fine to me 😄
Thanks for taking a look @etimberg. I've added a test |
src/core/core.controller.js
Outdated
meta.type = type; | ||
} else if (meta.type !== type) { | ||
meta.controller.destroy(); | ||
delete me.data.datasets[datasetIndex]._meta; |
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.
meta are scoped per chart (_meta[chart.id]
), each chart responsible to maintain their own meta data, so we should not delete all _meta
here. Also, will be better to move that code in a new private method destroyDatasetMeta
:
/**
* @private
*/
destroyDatasetMeta: function(datasetIndex) {
var id = this.id;
var dataset = this.data.datasets[datasetIndex];
var meta = dataset._meta && dataset._meta[id];
if (meta) {
meta.controller.destroy();
delete dataset._meta[id];
}
}
This method can also be used in chart.destroy
to replace lines 684 to 688.
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.
done
src/core/core.controller.js
Outdated
meta.controller.destroy(); | ||
delete me.data.datasets[datasetIndex]._meta; | ||
meta = me.getDatasetMeta(datasetIndex); | ||
meta.type = type; |
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.
Could be a bit simpler:
var type = dataset.type || me.config.type;
if (meta.type && meta.type !== type) {
me.destroyDatasetMeta(datasetIndex);
meta = me.getDatasetMeta(datasetIndex);
}
meta.type = type;
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.
done
test/specs/core.controller.tests.js
Outdated
|
||
// change the dataset to bar and check that meta was updated | ||
cfg.data.datasets[0].type = 'bar' | ||
chart.update(cfg); |
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.
update
doesn't accept the chart config as parameter but an update config object ({duration, easing, lazy}
)
I think I saw the same error in the financial example.
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.
done. updated the financial example as well
1cfe7a1
to
55579d5
Compare
Fix for #4585