Skip to content

Conversation

@jerairrest
Copy link
Collaborator

made it so we can dynamically update more than just the data

src/index.js Outdated
currentData[sid].data[pid] = nextData[sid].data[pid];
});

delete dataset.data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey,

Wouldn't this mutate the props ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically... yes.

But since we're building a new version of the dataset on every render for now this is sadly the only way of making sure we don't miss new properties that get passed in without doing a bunch of deepEquals. In the future I would like to make this more immutable, but for now this was the fastest way to get a fix out the door. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, I'm not sure we are building the new version of data set on each render.

Regardless, if we can just do:
const { data, ...dataset } = dataset instead of delete dataset.data;

I would be really happy 😃

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm.. sorry its early here. :-p Do you mean a complete line replacement of delete to your proposal like this?

nextData.forEach(function (dataset, sid) {
			if (currentData[sid] && currentData[sid].data) {
				currentData[sid].data.splice(nextData[sid].data.length);

				dataset.data.forEach(function (point, pid) {
					currentData[sid].data[pid] = nextData[sid].data[pid];
				});

				const { data, ...dataset } = dataset;

				currentData[sid] = {
					data: currentData[sid].data,
					...currentData[sid],
					...dataset
				}
			} else {
				currentData[sid] = nextData[sid];
			}
		});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

huh... I didn't even realize you could do that this way. Neat!

@gor181 gor181 merged commit 1a132ac into reactchartjs:master Dec 7, 2016
@gor181
Copy link
Collaborator

gor181 commented Dec 7, 2016

cheers mate!

@jerairrest jerairrest deleted the fix-dynamic-values branch December 7, 2016 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants