Skip to content

Make elements optional#6829

Merged
etimberg merged 1 commit intochartjs:masterfrom
benmccann:fastline-enablement
Dec 14, 2019
Merged

Make elements optional#6829
etimberg merged 1 commit intochartjs:masterfrom
benmccann:fastline-enablement

Conversation

@benmccann
Copy link
Contributor

This change would be necessary to allow the fastline controller from #6822 to be implemented externally

@benmccann benmccann mentioned this pull request Dec 11, 2019
kurkle
kurkle previously approved these changes Dec 12, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Ok, this is not required through, you could just set meta.data=dataset.data.

@kurkle kurkle dismissed their stale review December 12, 2019 09:38

I don't actually think this is a good idea

@kurkle
Copy link
Member

kurkle commented Dec 12, 2019

Edit: I'll have to think this more, it feels like we are going towards more trouble.

@kurkle
Copy link
Member

kurkle commented Dec 13, 2019

Ok, one issue here is that we can not know if the said controller creates equal amount of elements with the _parsed data, or if it uses _parsed at all.

So I don't think we should make elements optional. _parsed should be optional.

@benmccann
Copy link
Contributor Author

#6831 adds tests to make sure the arrays stay the same size, so I think it's okay to assume. But the code makes that assumption even without this PR by doing:

	item = metaData[i];
	parsed = meta._parsed[i];

This loop is primarily looping over and doing a calculation on _parsed so it makes most sense to use _parsed.length as the termination condition. We could do ilen = Math.max(meta._parsed.length, metaData.length) if you'd like to loosen the assumption that the two datasets are the same size though

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Fine, we can fix it later if its a problem.

@etimberg etimberg merged commit 9090b53 into chartjs:master Dec 14, 2019
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.

3 participants