yaph / d3-geomap

A library for creating geographical maps based on D3.js
https://d3-geomap.github.io/
MIT License
133 stars 45 forks source link

feat: build standard d3-module for d3v4+ & update examples #51

Closed dpraul closed 5 years ago

dpraul commented 5 years ago

With the release of version 4, d3 moved over to a modular approach, wherein everything is a plugin that extends off the main d3 module. In doing so, they setup a standard approach to building d3 modules. I originally recommended webpack for bundling source code, but the rest of the d3 universe uses rollup with a standard configuration (example for d3-shape).

Summarized, a standard d3-module for v4+ entails:

For convenience, I've added the following:

I've made several other small changes around the edges that I am happy to explain if you're wondering why they were made.

Below are the breaking changes. Do note, the API does not need to be changed, but I think making each of the exports distinct calls really clears up usage.

BREAKING CHANGES:

yaph commented 5 years ago

Big thanks for this PR @dpraul! I really appreciate your help and the effort you put into explaining and documenting your changes. They all look really good to me.

One additional change I want to make before the 3.0.0 release is to remove the colorbrewer component and use d3-scale-chromatic instead, i. e. colorbrewer.OrRd[9] would be replaced with d3.schemeOrRd[9] or an appropriate alias in choropleth.js. Since version 3 of d3-geomap will introduce breaking changes anyway, I think it's a good opportunity to remove this code. Please let me know if you think this is a bad idea.

If you are fine with this and want to update your PR with the removed code that would be awesome. Otherwise I can make the change after merging the PR. Provided you have no convincing arguments against it.

In any case I'm really happy to make progress with this and about your substantial contribution to this project.

dpraul commented 5 years ago

Whew I certainly let this fall away again, didn't I?

All is well and fine that you merged it, though, as my suggestion was to be that you do that extra migration in a separate PR, just for clearer git history, before making the 3.0.0 release.

I'll do a better job of keeping up with this from here on - we're not far from integrating this package into production, so it's in our best interest for me to do so, anyway 😄

yaph commented 5 years ago

Hey no problem. Not sure what you refer to by "extra migration" though. Do you mean my intended removal of colorbrewer? In any case, I haven't made any changes except for adding .github/FUNDING.yml from the master branch, which was not really necessary I guess.

dpraul commented 5 years ago

Yes, I was referring to the removal of colorbrewer when I said the "extra migration" - just me trying to keep pull requests closer to single-purpose. Are you still looking for assistance in removing colorbrewer?

yaph commented 5 years ago

I'd be happy if you send me another PR.