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

Google Bar, Scatter and Line cant be "required" twice on a page #995

Closed
jbrundage opened this issue Oct 5, 2015 · 24 comments
Closed

Google Bar, Scatter and Line cant be "required" twice on a page #995

jbrundage opened this issue Oct 5, 2015 · 24 comments
Assignees
Labels

Comments

@jbrundage
Copy link
Contributor

http://rawgit.com/hpcc-systems/Visualization/candidate-1.8.0/demos/dermatology.html?src/google/Bar

Go to the above link and toggle S. Test to see that it doesn't load the second instance of src/google/Bar

@mzummo
Copy link
Contributor

mzummo commented Oct 5, 2015

it will load 3rd and 4th though it seems

@mzummo
Copy link
Contributor

mzummo commented Oct 5, 2015

@GordonSmith can be tested via dermatology run serialization test and it wont run ... and then unclick and click again and it will run everytime after that .... is there a problem with the requirejs goog plugin?

@mzummo
Copy link
Contributor

mzummo commented Oct 6, 2015

only effects new 1.1 version of google visualization i believe

@mzummo
Copy link
Contributor

mzummo commented Oct 6, 2015

wasnt able to reproduce outside of VIZ ... something to do with our dataFactories and anonymous functions ... and only happens after loading google visualization 1.1

@mzummo
Copy link
Contributor

mzummo commented Oct 6, 2015

Mismatched anonymous define() module:

@mzummo
Copy link
Contributor

mzummo commented Oct 6, 2015

if ( typeof define === "function" && define.amd && define.amd.jQuery ) {
define( "jquery", [], function () { return jQuery; } );
}

using a named module above i dont know how that works (cant get it working) ... but maybe that might fix it?

@jbrundage
Copy link
Contributor Author

@GordonSmith This is currently causing Travis CI to fail

@GordonSmith
Copy link
Member

@jbrundage @mlzummo Nothing obvious I can spot (but I can reproduce) - the material charts are still in beta, so we may have no control over it (who issued this PR - did you know they were still beta?)

@mzummo
Copy link
Contributor

mzummo commented Oct 7, 2015

@GordonSmith I am not 100% sure but i dont think the issue is 100% google as i can only reproduce the issue when using the new DataFactory (so far that is)

@dtsnell4
Copy link

dtsnell4 commented Oct 7, 2015

I issued and I did not realize. #927

@dtsnell4
Copy link

dtsnell4 commented Oct 7, 2015

Should we revert back to old version and forgo dual y axis for now?

@mzummo
Copy link
Contributor

mzummo commented Oct 7, 2015

@dtsnell4 is there any reason we can't revert or did the new version provide a feature we needed?

@GordonSmith
Copy link
Member

@jbrundage @mlzummo @dtsnell4 I think it may just be a async loading issue and that Line 1.1 needs corechart to be loaded first - investigating...

GordonSmith added a commit to GordonSmith/Visualization that referenced this issue Oct 7, 2015
I think material chart needs core charts to be fully loaded before its loaded.
While this fix appears to work, I am not sure why the corechart in the base CommonND is not being loaded first.

Fixes hpcc-systemsGH-995

Signed-off-by: Gordon Smith <[email protected]>
@mzummo
Copy link
Contributor

mzummo commented Oct 7, 2015

its no longer happening in dermatology

@jbrundage
Copy link
Contributor Author

It's occasionally still happening in dermatology

GordonSmith added a commit to GordonSmith/Visualization that referenced this issue Oct 7, 2015
Ensure each class only loads what is actually required.

Fixes hpcc-systemsGH-995

Signed-off-by: Gordon Smith <[email protected]>
@GordonSmith
Copy link
Member

well it was nice that travis was catching it (even with the d3-cloud issue).

@mzummo
Copy link
Contributor

mzummo commented Oct 15, 2015

https://gist.github.com/Integralist/1599546 take a look at this

@mzummo
Copy link
Contributor

mzummo commented Oct 15, 2015

@mzummo
Copy link
Contributor

mzummo commented Oct 15, 2015

@mzummo
Copy link
Contributor

mzummo commented Oct 15, 2015

@mzummo
Copy link
Contributor

mzummo commented Oct 15, 2015

i think i know how to fix this but its going to require a pretty big refactor.

@GordonSmith perhaps after i explain you can help figure out a way to minimize the code required?

@mzummo
Copy link
Contributor

mzummo commented Oct 15, 2015

    define(["d3", "../common/HTMLWidget", "async!http://www.google.com/jsapi"], factory);

...
google.load("visualization", 1.1, {"packages":["bar"]});
google.setOnLoadCallback(drawChart);
context._chart = new google.charts.Bar(domNode);

@GordonSmith

@GordonSmith
Copy link
Member

That is effectively what goog! does: https://github.com/GordonSmith/requirejs-plugins/blob/master/src/goog.js

@mzummo
Copy link
Contributor

mzummo commented Oct 15, 2015

nts: perhaps looks at r.js?

GordonSmith added a commit to GordonSmith/Visualization that referenced this issue Dec 14, 2015
Hack to work around google webfont issue:  typekit/webfontloader#278

Fixes hpcc-systemsGH-995

Signed-off-by: Gordon Smith <[email protected]>
GordonSmith added a commit to GordonSmith/Visualization that referenced this issue Dec 14, 2015
Hack to work around google webfont issue:  typekit/webfontloader#278

Fixes hpcc-systemsGH-995

Signed-off-by: Gordon Smith <[email protected]>
GordonSmith added a commit that referenced this issue Dec 17, 2015
GH-995 RequireJS + Google Material Charts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants