Skip to content
This repository was archived by the owner on Jul 19, 2019. It is now read-only.

v2: Assign all nextProps dataset properties to chart #109

Merged
merged 5 commits into from
May 18, 2016
Merged

v2: Assign all nextProps dataset properties to chart #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f6c340
v2: assign all nextProps dataset properties to chart
ianks Apr 19, 2016
15059bc
v2: Simplify chart type logic
ianks Apr 19, 2016
1da6721
Require chart.js at beginning of module
ianks May 4, 2016
ad46cca
Fixes chart creation on initializeChart
eluciano11 May 6, 2016
0cedffb
Merge pull request #1 from eluciano11/label-fix-v2
ianks May 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dist/react-chartjs.min.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 23 additions & 36 deletions lib/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

var React = require('react');
var ReactDOM = require('react-dom');
var Chart = require('chart.js');

module.exports = {
createClass: function(chartType, methodNames, dataKey) {
Expand Down Expand Up @@ -48,51 +49,37 @@ module.exports = {
classData.componentWillReceiveProps = function(nextProps) {
var chart = this.state.chart;

// // Reset the array of datasets
chart.data.datasets.forEach(function(set, setIndex) {
set.data.forEach(function(val, pointIndex) {
set.data = [];
});
});

// // Reset the array of labels
chart.data.labels = [];
if (nextProps.redraw) {
chart.destroy(); // Reset the array of datasets
this.initializeChart(nextProps);
} else {
// assign all of the properites from the next datasets to the current chart
nextProps.data.datasets.forEach(function(set, setIndex) {
var chartDataset = chart.data.datasets[setIndex];

// Adds the datapoints from nextProps
nextProps.data.datasets.forEach(function(set, setIndex) {
set.data.forEach(function(val, pointIndex) {
chart.data.datasets[setIndex].data[pointIndex] = nextProps.data.datasets[setIndex].data[pointIndex];
for (var property in set) {
if (set.hasOwnProperty(property)) {
chartDataset[property] = set[property];
}
}
});
});

// Sets the labels from nextProps
nextProps.data.labels.forEach(function(val, labelIndex) {
chart.data.labels[labelIndex] = nextProps.data.labels[labelIndex];
});
chart.data.labels = nextProps.data.labels;

// Updates Chart with new data
chart.update();
chart.update();
}
};

classData.initializeChart = function(nextProps) {
var Chart = require('chart.js');
var el = ReactDOM.findDOMNode(this);
var ctx = el.getContext("2d");
var type = (chartType === 'PolarArea') ? 'polarArea':chartType.toLowerCase();

if (chartType === 'PolarArea'){
var chart = new Chart(ctx, {
type: 'polarArea',
data: nextProps.data,
options: nextProps.options
});
} else {
var chart = new Chart(ctx, {
type: chartType.toLowerCase(),
data: nextProps.data,
options: nextProps.options
});
}
this.state.chart = chart;
this.state.chart = new Chart(ctx, {
type: type,
data: nextProps.data,
options: nextProps.options
});
Copy link

Choose a reason for hiding this comment

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

Would it be better to use setState here? Thank you for this commit!

Copy link
Contributor

Choose a reason for hiding this comment

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

@dougmolineux I would agree.
Even moving this.initializeChart from componentDidMount to componentWillMount to avoid unecessary re-render.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think this is possible since initializeChart relies on the chart being mounted.

Copy link
Author

Choose a reason for hiding this comment

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

It also might not be possible to use setState since it is asynchronous, and the chart being available is required to be sync the way the code is currently written. I'd rather not turn this PR into a full re-factor.

};


Expand All @@ -118,4 +105,4 @@ module.exports = {

return React.createClass(classData);
}
};
};