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 lineChart.interpolate -> lineChart.curve in a backward-compatible way #1376

Closed
gordonwoodhull opened this issue Mar 22, 2018 · 4 comments
Milestone

Comments

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Mar 22, 2018

Since d3 has replaced d3.svg.line.interpolate with d3.line.curve, we should too. But we have the opportunity to do it in a mostly backward-compatible way.

First, we can create the new lineChart.curve() which takes a curve object, but leave lineChart.interpolate(), still taking a string but instantiating the right curve object based on the old names of interpolations. We should deprecate interpolate using dc.logger.deprecate, and remove it in 3.1 or later.

@kum-deepak, I think we can leave tension the way you have implemented it in _interpolateWithTension, but instead of silently dropping tension when it's not applicable, we should warn the user in the browser console, "Tension specified for a curve that doesn't support it, ignored"

@kum-deepak
Copy link
Collaborator

This is a good idea. It has some additional benefit. See below.

I also wanted to change the behavior of tension a little bit. In the previous version the default value was 0.7 while this made the output look different, after changing to 0 it looks similar. So, in all likelihood interpretation of tension has changed within D3.

I was thinking to set value of default tension to null instead of 0. That way if someone did not set it we do not try to apply it.

This also has one benefit that one can apply the tension to the d3 curve function before passing it to chart.curve (like chart.curve(d3.curveCardinal.tension(0.5))). As tension will not be reapplied, it will work as expected.

@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Mar 22, 2018

That's great. Maybe we should deprecate tension too, since that looks like a more expressive, idiomatic way to customize the curve.

@gordonwoodhull gordonwoodhull added this to the 3.0 milestone Mar 22, 2018
@kum-deepak
Copy link
Collaborator

Great, will make these changes and raise a PR.

@gordonwoodhull
Copy link
Contributor Author

fixed by #1381

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

No branches or pull requests

2 participants