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

Fix update prototype sample code #3891

Closed
wants to merge 1 commit into from

Conversation

kennethkalmer
Copy link

Sample JS snippet incorrectly shows myLineChart.data being updated instead of myLineChart.config.data.

Sample JS snippet incorrectly shows `myLineChart.data` being updated instead of `myLineChart.config.data`.
@etimberg
Copy link
Member

I'm not sure this is wrong, changing chart.data should work as well

@kennethkalmer
Copy link
Author

kennethkalmer commented Feb 10, 2017

@etimberg thanks for the response. I spent 2 hours battling with it thinking I didn't understand how ClojureScript & JavaScript interop worked... Then I had a cursory look at the samples and saw that they mostly use config.data, and suddenly it all worked.

@simonbrunel
Copy link
Member

@kennethkalmer do you have an example that show your issue? I tried to update a chart using chart.data and it works fine. chart.data is an alias of config.data, so both should work the same but chart.data is the preferred form. The only difference is that you can't assign a new object to chart.data (e.g. chart.data = { ... }), we still need to define a setter.

Duplicated of #3785

@kennethkalmer
Copy link
Author

@simonbrunel Aha! That makes sense, I was blatantly reassigning chart.data with chart.data = {labels: [], datasets: []}. So until the setter is made I recommend the documentation gets modified to indicate this (or even updated regardless for people using the versions prior to the version getting the setter).

I can take a stab at improving it in this way if you'd like?

@simonbrunel
Copy link
Member

@kennethkalmer I'm going to implement the setter with unit tests but also try to clean up examples to use chart.data instead of config.data. I can also update the doc with a note about chart.data = {} not working for version prior 2.6 (I don't think we will get the doc re-deployed before then :).

@etimberg thoughts?

@simonbrunel
Copy link
Member

@kennethkalmer closing in favor of #3897, let me know your thoughts about that changes.

@kennethkalmer kennethkalmer deleted the patch-1 branch February 11, 2017 15:49
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.

3 participants