search

mgood / js.d3

Fanstatic package for D3.js
0 stars 4 forks source link

Upgrade D3 to v3.1.5 and add conditional comment to restrict D3 to IE>=9 #1

Closed davidjb closed 10 years ago

davidjb commented 11 years ago

This pull request upgrades the D3 library to the latest version at time of writing, expands the readme documentation a little, and adds a conditional comment to restrict IE 8 or lower from loading D3. Without this conditional comment, even simply loading the D3 JS produces errors as per https://github.com/mbostock/d3/issues/619 - thus breaking the rest of JavaScript on a given page.

If you're happy to accept this, then could you please make a new release of js.d3? Otherwise, I'm happy to help maintain this package.

mgood commented 11 years ago

Thanks, sorry for the delay.

I'm glad to go ahead and merge the version upgrade, though I have a few questions about the IE conditional inclusion first.

If the d3 script is conditionally included, how do you handle the fallback when it's missing? I guess you can test for if (d3), but I'd like some recommendations or notes on how to handle this on IE8. I also have a js.rickshaw package that I would need to figure out how to make compatible with this change, since it would cause cascading failures when d3 is not included.

It also seems like the conditional comments will be bypassed if you create a bundle of assets, since the d3 JS would be combined with other scripts.

davidjb commented 11 years ago

No worries.

The IE8 issue is a touchy issue as if the D3 script is loaded at all, then at least 2 errors occur and then JS is broken on the page. D3 itself mentions nothing about this use case except for a few mailing list posts about it, and the suggestion (iirc) was to conditionally include D3 onto the page in the first place (eg checking for browser support), and then continuing on. Personally, I think this is something that D3 should check for before initialising itself to avoid breaking the rest of the page, but that's another matter. The IE8 condition was about as simple as this could be made save for adding something like SVG checks, depending on Modernizr etc.

The fallback I'd taken to in my code that depends on d3 is to check if it isn't undefined (eg if (typeof d3 !== 'undefined') {...}) and carry on. As for dependent libraries/packages, my thinking is that those packages should either use the same IE8 conditional comment (import from js.d3; or implement their own, depending on the functionality of the JS library).

As for the bundle issue, if my understanding is right, I think it won't be an issue since bundles are restricted to within the same Fanstatic library (eg js.d3 will only get bundled with resources within js.d3), and js.d3 only has the one file within it for now. I guess this would need re-visiting if d3 add more resources.

davidjb commented 11 years ago

Any chance of this pull request being merged? It'd be nice to see a release of the newest version of this library. Pull request updated to version 3.1.5 from 3.0.5.

there4 commented 11 years ago

+1. I'm currently having to package around this, and it would be great to have this included.

lisp-ceo commented 11 years ago

+1 I'm having to package around this too.

mgood commented 10 years ago

Ok, I've updated with the current 3.3.13 version of D3 (still need to upload a new release).

However, it looks like for browser support, D3 suggests using Aight. Or, there's R2D3 which provides a compatibility shim based on Raphael. So, I'm still torn about the IE8 issue, since it doesn't seem like the conditional comment is going to be the best solution for everyone. I've opened issue #2 to continue discussing IE8.