keen / keen-js

https://keen.io/ JavaScript SDKs. Track users and visualise the results. Demo http://keen.github.io/keen-dataviz.js/
MIT License
578 stars 143 forks source link

'Library you selected does not support this chartType' in clean-viz #149

Closed dok closed 10 years ago

dok commented 10 years ago

Hey @aroc,

I ran the manual test in test/examples/visualize/index.html and the refactor seems to be causing some issues on selecting the chartType if the user does not specify it. My assumption is that none of those existing tests should be failing except for the intentional one. Could you take a look and let me know what you think? Thanks!

I've attached a screenshot with the stacktrace

screen shot 2014-09-02 at 10 32 32 am

aroc commented 10 years ago

@Dokko1230 Yep I'll take a look!

aroc commented 10 years ago

@Dokko1230 Seems to work as expected for me. Are you running the clean-viz branch and did you pull down any changes?

dok commented 10 years ago

@aroc Oh! Issues seem to be on my side. Simply forgot to turn on grunt watch..

Closing. Sorry about that :O

dok commented 10 years ago

Seems that the reason for this was that the trigger method for this.visual in _build_visual was taken out. And the merge did not successfully cover that scenario for me.. Weird.

https://github.com/keenlabs/keen-js/blob/clean-viz/src/visualize.js#L67

vs.

https://github.com/keenlabs/keen-js/blob/master/src/visualize.js#L81

aroc commented 10 years ago

@Dokko1230 I may have missed something when manually cherry-picking changes form master and merging them into the clean-viz branch. We can totally add that event triggering back in.

dok commented 10 years ago

Gotcha. I added it in the clean-viz branch to test it out. Once I add it, it errors with the same stacktrace for me.

Stacktrace: screen shot 2014-09-02 at 11 55 40 am

aroc commented 10 years ago

@Dokko1230 How are you using the lib? When does the lack of that trigger call cause problems? I'm just trying to conceptualize a potential fix.

dok commented 10 years ago

I'm simply running code in index.html

It seems the issue seems to exist when this example is run. This error exists because the trigger method does not exist at this runtime for this.visual. Perhaps it is being overridden? Or maybe it is just not being extended to DataViz?

aroc commented 10 years ago

@Dokko1230 I don't think I'm understanding. You're just running the test/examples/visualize/index.html file? I can't replicate the error.

dok commented 10 years ago

Yep. I am just running that file.

Reproduction steps:

  1. Checkout the clean-viz repository
  2. On line 67 of visualize.js, add in the line
if (this.visual) {
      this.visual.trigger("remove");
 }
  1. On chrome, refresh test/examples/visualize/index.html file
aroc commented 10 years ago

@Dokko1230 Oh ok, I didn't realize you added the line in order to get the error. To clarify, do you require the this.visual.trigger("remove"); code in the _build_visual function?

dok commented 10 years ago

Yes that is correct.

if (this.visual) {
  this.visual.trigger("remove");
}
aroc commented 10 years ago

Ok cool. I'll find some time today to look into how to bring that back.

aroc commented 10 years ago

@Dokko1230 Ok, I think this should be working now on the clean-viz branch. Let me know if you have any other issues.

dok commented 10 years ago

Looks fixed. Thanks!