-
Notifications
You must be signed in to change notification settings - Fork 131
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
Pluginize #30
Pluginize #30
Conversation
…lugin/ - Created rollup config file - Using npm for dependencies - Adopt ES6 module style imports Updated example
Updated usage instructions
Thank you so much for this, @ale0xb. I likely won't be able to review it for a couple of weeks, but as soon as I have a chance to, I will. Feel free to bump this with a comment if it languishes for too long. 😬 |
Bump! 👋 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this, @ale0xb! I have just a couple of minor changes.
index.html
Outdated
// map.call(zoom); | ||
updateZoom(); | ||
layer.attr("transform", | ||
"translate(" + -38 + "," + 32 + ")" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit-picky thing: can this also be an array stringification, a la [-38, 32]
? This will make it easier to turn the translation into a single variable.
index.html
Outdated
updateZoom(); | ||
layer.attr("transform", | ||
"translate(" + -38 + "," + 32 + ")" + | ||
"scale(" + [0.94, 0.94] + ")"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"scale(" + 0.94 + ")"
would suffice here, and also make it easier to update the scale by turning it into a variable like the translation distance.
If you have the time, would you also mind trying to merge @adamflorin's changes in #28 into this PR? Thank you!! |
Hello! Regarding @adamflorin PR I think most of the functionality was already implemented. This library is using a rollup setup that produces umd builds (and thus makes the code compatible with node and the browser). I didn't have time to test it myself but theoretically it should work just as fine as the official d3 plugins do (ie: d3-scale). Trigger a build and look at the first lines of build/d3-cartogram.js, you'll see what I mean. Anything else let me know, happy to help! |
Hello again @shawnbot, just a quick heads up regarding this PR. Anything else missing? |
Beautiful, thank you @ale0xb! I'll publish a release to npm shortly with you listed as a contributor. |
Lovely @shawnbot, thank you very much! |
This PR does several things:
index.html
package.json
Theoretically with very few further actions people could use this code by running
npm install d3-cartogram
in their projects. Any doubts, let me know. Thanks!