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
austinpray merged 5 commits into from
May 18, 2016
Merged

v2: Assign all nextProps dataset properties to chart #109

Changes from 2 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
55 changes: 20 additions & 35 deletions lib/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,51 +48,36 @@ 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');
Copy link
Contributor

@gor181 gor181 May 2, 2016

Choose a reason for hiding this comment

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

Is there are certain penalty of doing this require as the initializeChart method is called on componentWillReceiveProps?

Copy link
Author

@ianks ianks May 4, 2016

Choose a reason for hiding this comment

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

I'm doubtful, but for the sake of clarity I'll move it up

var el = ReactDOM.findDOMNode(this);
var ctx = el.getContext("2d");

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 = Chart[chartType](ctx, {
data: nextProps.data,
options: nextProps.options
});
Copy link

@dougmolineux dougmolineux May 2, 2016

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

@gor181 gor181 May 2, 2016

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

@ianks ianks May 4, 2016

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

@ianks ianks May 4, 2016

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 +103,4 @@ module.exports = {

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