search

reactjs / react-chartjs

common react charting components using chart.js
MIT License
2.93k stars 301 forks source link

CommonJS Changes #5

Closed seanadkinson closed 9 years ago

seanadkinson commented 9 years ago

Obviously you don't want this PR as-is (since the README is changed), but let me know what you think about the vars.js -> createClass.js changes, since that was really the area that wasn't working for me.

jhudson8 commented 9 years ago

I think these changes are going in the right direction.

The name change is fine - I had it with that name because it was where the react instance was set previously I'm find with referencing the modules through a central entry point rather than the sub modules directly I do not want to remove support for AMD though - think you can get that straight?

seanadkinson commented 9 years ago

Yeah, let me look into that.

Sent from my iPhone

On Jan 29, 2015, at 5:36 AM, Joe Hudson notifications@github.com wrote:

I think these changes are going in the right direction.

The name change is fine - I had it with that name because it was where the react instance was set previously I'm find with referencing the modules through a central entry point rather than the sub modules directly I do not want to remove support for AMD though - think you can get that straight?

— Reply to this email directly or view it on GitHub.

jhudson8 commented 9 years ago

depending on the difficulty, I'm not opposed to just supporting CommonJS - but it would be ideal to support all.

seanadkinson commented 9 years ago

Haven't been able to look at this yet, but might have a chance today or tomorrow.

jhudson8 commented 9 years ago

see comments on https://github.com/jhudson8/react-chartjs/issues/4