d3plus / d3plus

A javascript library that extends D3.js to enable fast and beautiful visualizations.
MIT License
1.62k stars 188 forks source link

IE 9 Bugs #171

Closed danihodovic closed 9 years ago

danihodovic commented 9 years ago

I've been testing my project through different browsers and it seems to work fine in Firefox and Chrome. However it has issues when it comes to IE, and specifically IE9. Do you have any plans on supporting IE9 at all? If you were to implement this I would gladly and actively help out with bug tracking.

davelandry commented 9 years ago

Yes, we need IE9 support as well for some of our internal projects. Please post any IE9 bugs you find!

alexandersimoes commented 9 years ago

The author of d3 recommends using aight for HTML5 shim/polyfill bundle for compatibility.

Since D3plus is built on top of D3 it's held to the same browser restrictions see here - https://github.com/mbostock/d3/wiki.

davelandry commented 9 years ago

I believe aight works as a shim for IE8, not IE9.

D3plus worked with IE9 in the past, so these bugs @dani-h is speaking of are probably just related to not enough user testing with the new releases in Internet Explorer.

danihodovic commented 9 years ago

D3Plus v1.5.1, IE9 (Browserstack)

SCRIPT5007: Unable to get value of the property 'value': object is null or undefined d3plus.1.5.1.js, line 26672 character 7

Lines 26671-26673: if ( "data" in vars && vars.data.value && d3plus.util.d3selection( vars.data.value ) ) { vars.data.value = parseElement( vars ) }

Note: The line numbers in the official release and my local release may differ due to a simple data-id injection I have in the source.

danihodovic commented 9 years ago

I've briefly tested 1.6 on Chrome and Firefox and the treemap doesn't display any names for the boxes. They have a size, color and value, but are nameless. Has the API changed?

davelandry commented 9 years ago

Did you compile a new build of d3plus.js? If not, please pull my most recent commit and check again. Labels are rendering correctly for me.

We don't always push/commit the compiled version of the library to pre-release branches, so you may have been using an out-of-date (broken) build.

danihodovic commented 9 years ago

I pulled your most recent commit, compared commit logs with Github and built using gulp. It still doesn't display in Chrome. It could be something I'm doing wrong, but it seems like I'm loading the most recent file.

danihodovic commented 9 years ago

What is the difference between d3plus.js and d3plus.full.js?

alexandersimoes commented 9 years ago

d3plus.full.js includes a copy of d3.js so you wouldn't need to include both.

danihodovic commented 9 years ago

Thanks!

Edit: Using the d3plus.full.js found on the 1.6 branch breaks.

Uncaught ReferenceError: d3 is not defined, line 20032

danihodovic commented 9 years ago

After closer testing I realize that viz.id() and viz.text() do not bind to the elements at all.

davelandry commented 9 years ago

Thanks for catching this, it was an issue that was the result of our move to full Browserify require statements. The global d3 object was not being declared before some of the other files were instantiating. It's fixed in my latest commit.

danihodovic commented 9 years ago

No problem, thanks for fixing it.

However, the treemap still doesn't bind values via viz.id(val) & viz.text(val)

davelandry commented 9 years ago

What do you mean by "bind values"? Can you post some sample code?

danihodovic commented 9 years ago

I was wrong, I forgot to add data attributes to the svg elements by altering the source...

It seems like the labels still don't show up. I've tried both building with gulp and using your most recent build on branch 1.6. I'll look a closer look at the problem today.

danihodovic commented 9 years ago

Well I took a closer look at why the labels weren't showing up for me in build 1.6.

I had a look at 5-6 different files but centralized the issue around finish.js, labels.js and text.js, and specifically text.js as the others are dependent on the return of [names] from here.

Lines 34-37 in text.js: if (validObject(obj) && "d3plus" in obj && obj.d3plus.text) { names.push(obj.d3plus.text.toString()) names.push(vars.format.value(obj.d3plus.text.toString(), undefined, vars)) }

obj.d3plus.text is always undefined and a closer look reveals that obj.d3plus only contains Object { depth: 0, active: 0, temp: 0, total: 1, x: 889.5, y: 583, width: 97, height: 54, share: 0.0074219103840505885, shape: "square", 2 more… } (2 more = id, segments) It's missing the text attribute.

When creating d3plus.viz I called d3plus.viz.text("component_name"). This attribute is found in the obj. If I call names.push(obj.component_name) in text.js (line 33, just before the if statement) it seems to work fine and the labels are displayed properly.

I figure that theres a deeper issue in that obj.d3plus.text doesn't get assigned the keyvaluepair {"component_name": "somecomeponent"} or it doesn't get assigned the "component_name" value period.

I could have poked the source even further, but I think you know it much better than me and will find the solution much faster.

Off topic: Out of curiousity, why do the modules only contain one function each? It seems like the code gets very cluttered and non-modular with one function spanning through several hundreds of lines of code.

davelandry commented 9 years ago

Thanks for the help, it should be fixed now. Please do a pull and check.

I hear you on the messy code. There is still a lot of cluttered/bad code that we consider "legacy" code that needs to be re-written, but in general we're pretty modular with anything new we've been writing. Is there a specific function you're referring to in particular?

danihodovic commented 9 years ago

All the three files I looked at, and a couple of others were basically all of the functionality contained within the export function.

I'm not here to bash any style of programming, maybe this way of building d3plus is best for you, I'm simply not used to it. But I think that contributors need a fair amount of time and effort to get into the codebase, especially if the files are basically if/else directives through hundreds of lines of code.

davelandry commented 9 years ago

I agree with that, and that's why we're moving away from working that way. It's a relic of when the library was much, much smaller and had much less functionality. Features were being very rapidly added with if/else statements to meet a deadline we had, which led to the confusing codebase.

Unfortunately, it just takes a lot of time to re-write code!

danihodovic commented 9 years ago

If I have a lot of time on my hands I might rewrite a few modules and make pull requests.

Are you moving over to Coffeescript entirely? I guess this is the target language for refactoring?

How will you cope with the ES6 release though (June 2015?)? I'm saying this because the Coffeescript community seems to be focusing on backwards compatibility to Javascript, which means that they won't support the new features of ES6. It would suck having to rewrite a codebase of this size twice. On the other hand I do love Coffeescript and for any small personal project I would probably use it too, despite the ES6 release.

davelandry commented 9 years ago

We'd love your help with the codebase!

I've been converting some of the code to Coffeescript where I feel it makes sense, but I'm ok with the project supporting both Coffeescript and Javascript (@alexandersimoes hasn't fully jumped ship to Coffeescript yet :wink:).

I personally love Coffeescript because I find it easier to read (no enclosures), and it helps catch some common mistakes (semi-colons and variable scoping).

I haven't spent much time reading about ES6, and I'm not too worried about re-writing things for it. If there are some new features that make sense to incorporate, then sure, but I want to make sure we still support some older browsers (or at least have some sort of converter to ES5 when compiling).

danihodovic commented 9 years ago

ES6 has a couple of interesting features going on. Most relevant to this project would probably be the module system (no need for require.js/browserify), classes and generators.

I'd like to implement the ability to data bind using HTML5's data attributes. With this you can identify any specific svg elements according to your backend schema. It makes it fairly easy to add mouse clicks, manipulate the visualization etc. You can probably bind the entire data object :smiling_imp: .

Where is the public interface to d3plus, i.e d3plus.viz located? Do you have some sort of class or structure diagram? Edit: I can probably grep this

davelandry commented 9 years ago

I'm not sure using HTML5's data attributes and having the user have to manually select elements is the right way to go. This type of functionality should be implemented as public methods, where the user would have access to something like .mouse( ).

We're planning on full internal documentation, a structure diagram, and guides for adding functionality (but unfortunately aren't at that stage yet).

I think it would be beneficial for us to jump on a Skype chat, shoot me an e-mail: landry.dave@gmail.com

danihodovic commented 9 years ago

Yeah, for simplicity mouse events may be the way to go. I think binding d3 .data to the DOM, or giving access to some sort of coupling between the provided data and the generated elements might open up for a lot of creative ideas. As it is right now d3plus is very sandboxed and outside of the public api theres not much you can do really.

I'll shoot you an email later, I'm off for tonight