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

Allow updating dataset types #4586

Merged
merged 1 commit into from Aug 2, 2017
Merged

Conversation

benmccann
Copy link
Contributor

Fix for #4585

@benmccann
Copy link
Contributor Author

This would allow me to remove the hack in #4554

Copy link
Member

@etimberg etimberg left a 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 😄

@benmccann
Copy link
Contributor Author

Thanks for taking a look @etimberg. I've added a test

meta.type = type;
} else if (meta.type !== type) {
meta.controller.destroy();
delete me.data.datasets[datasetIndex]._meta;
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

meta.controller.destroy();
delete me.data.datasets[datasetIndex]._meta;
meta = me.getDatasetMeta(datasetIndex);
meta.type = type;
Copy link
Member

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// change the dataset to bar and check that meta was updated
cfg.data.datasets[0].type = 'bar'
chart.update(cfg);
Copy link
Member

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.

Copy link
Contributor Author

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

@benmccann benmccann force-pushed the meta-cache branch 2 times, most recently from 1cfe7a1 to 55579d5 Compare August 1, 2017 17:29
@benmccann benmccann mentioned this pull request Aug 2, 2017
@simonbrunel simonbrunel merged commit 2922dc9 into chartjs:master Aug 2, 2017
@simonbrunel simonbrunel added this to the Version 2.7 milestone Aug 14, 2017
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants