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

Fix for nice() at certain scales. #104

Merged
merged 2 commits into from May 9, 2017
Merged

Fix for nice() at certain scales. #104

merged 2 commits into from May 9, 2017

Conversation

danielyule
Copy link
Contributor

The fix for #9 was causing me some problems with scale. I was getting the following behavior, which I'm pretty sure isn't expected:

const scale = d3.scaleLinear().domain([0, 14.1]).nice(5);
scale.domain(); // [0, 15]
scale.ticks(5); // [0, 2, 4, 6, 8, 10, 12, 14]

I was essentially having the reverse problem to the other users in in the issue. When stepping through, nice() was selecting a step size of 2 in its first calculation, then going back to 5 for its second (and thus not updating the domain). However, ticks() was calculating 2 as its step size, and therefore only going up to 14.

These are not the only numbers where I was seeing this problem, I just picked these ones in particular for an example.

The fix here updates the domain twice, instead of only updating the step size twice. I think it fixes my problem, without adversely affecting any other situations. In the example I gave, we now have

const scale = d3.scaleLinear().domain([0, 15]).nice(5);
scale.domain(); // [0, 20]
scale.ticks(5); // [0, 5, 10, 15, 20]

I think under ideal circumstances we'd have,

scale.domain([0, 15]).ticks(5) // [0, 5, 10, 15]

But changing that would require changing the tick error constants, and I'm guessing there's a reason they are the way they are.

danielyule and others added 2 commits May 9, 2017 10:04
nice() was calculating the step value twice, but using the initial
endpoints to calculate the final domain.  Instead, it now
updates the endpoints each time after calculating the step.
@mbostock
Copy link
Member

mbostock commented May 9, 2017

I’ve revised your implementation slightly, but it should be functionally equivalent. Seems like a reasonable fix to me. Thank you!

@mbostock mbostock merged commit 4e2ed12 into d3:master May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants