Skip to content
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

Upgrade to d3v4 #9

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

basilesimon
Copy link
Collaborator

@basilesimon basilesimon commented May 22, 2017

This pull request upgrade d3-grid code to d3v4.

This is still a work in progress and should not be merged yet

Goals

Upgrade to d3v4 : e2714ad

x = d3.scale.ordinal(), becomes x = d3.scaleOrdinal(),

x.domain(d3.range(_cols)).rangeBands become

var x = d3.scaleBand();
 x.domain(d3.range(_cols)).range([0, size[0]], padding[0], 0);

x.domain(d3.range(_cols)).rangePoints becomes

var x = d3.scalePoint();
x.domain(d3.range(_cols)).range([0, size[0]]);

Trim down the dependencies to modular d3: 838c964

The plugin now only loads d3-scale, instead of the whole of d3.

  "dependencies": {
    "d3-scale": "^1.0.6"
  },

Adopt d3v4 plugin structure: f3015dd

d3-grid.js has been moved to a src/ directory. See guidance on publishing a d3 plugin

@basilesimon basilesimon self-assigned this May 22, 2017
@gka
Copy link

gka commented Jul 11, 2017

👍

@basilesimon
Copy link
Collaborator Author

Thanks @gka - we're waiting on @herrstucki to merge this in!
That said, I've used it successfully in prod for the Times general election results page: https://www.thetimes.co.uk/electionresults (resize to < 768px across)

Copy link
Contributor

@jstcki jstcki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is terrific. I haven't had time to test it yet, but from reading the source, it all looks good.

One thing though: you'll need to add a build step for consumers which can't import ES modules directly. See https://github.com/d3/d3-selection/blob/master/package.json#L26 for example. Also make sure that the respective module types are referenced properly in package.json, e.g. https://github.com/d3/d3-selection/blob/master/package.json#L18-L20

package.json Outdated
"main": "d3-grid.js",
"scripts": {
"test": "node_modules/.bin/vows --spec"
},
"repository": "https://github.com/interactivethings/d3-grid.git",
"repository": "https://github.com/basilesimon/d3-grid.git",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@herrstucki done 👍

@jstcki
Copy link
Contributor

jstcki commented Jul 12, 2017

@basilesimon not sure if you're still working on it but the package.json was left broken in your last commit and the dependencies are missing. Please make sure the tests actually pass and the build process works before you push 😉 Thanks in advance! 🙇

P.S.: I fixed package.json, so that it's at least parseable again
P.P.S.: I've also enabled Travis, so we get test feedback here as well.

@basilesimon
Copy link
Collaborator Author

@herrstucki Ah, this is about as far as I'm going to go. Some tests are still soft-failing (not being return what's expected)... I'd really welcome your help on these at this point.

The build and packaging have been fixed and now working nicely 💯 😀

@oller
Copy link

oller commented Jan 9, 2018

How's this looking guys? Hit a stumbling block on an v3 to v4 upgrade with this handy dependency!

@basilesimon
Copy link
Collaborator Author

@oller The code in this PR is in use in production on my side. I just can't quite get it to pass the tests, hence the PR being still open...
I recommend you try it out and give me a shout if you run into trouble, I'll do my best to help 👌

@oller
Copy link

oller commented Jan 9, 2018

Ah great, I'll give it a whirl. Thanks for the prompt response @basilesimon! 👍

@oller
Copy link

oller commented Jan 9, 2018

How are you importing and referencing this lib now @basilesimon, as d3 v4 has done away with d3.layout.grid which this plugin used to extend.

I can see the plugin present in node_modules with the PR code. I've tried both...

import d3grid from "d3-grid";
import d3grid from "d3-grid/index.js";

Both return undefined, the only way I can get some sense back is:

import d3grid from "d3-grid/src/d3-grid.js" but then that errors citing the d3 referenced within is not defined.

I've tried using both yarn and npm to install this PR in their respective formats. I'm on webpack 3.4

Any insights appreciated.

@basilesimon
Copy link
Collaborator Author

@oller that takes a bit of fiddling.

See this folder for example: I've adapted d3-grid.js inside layout.js to make it easier to import, although as you can see there's no Webpack in there...

@basilesimon
Copy link
Collaborator Author

Hey @herrstucki, sorry to bother you again, but I think I need some help to get this over the line.

It would be fantastic to have this plugin properly available. At the moment this is something I personally use all the time, but the implementation described in my above comment is pretty gross.

Also, we now have this fork with a confusing name (cc @finnfiddle).

Maybe it would be simpler if we had one, up-to-date version of this plugin we can all contribute to?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants