Ensure Graph creates a copy of the x/yScale before modifying it #387

Merged
merged 1 commit into from Apr 13, 2014

5 participants

@rbu
rbu commented Feb 5, 2014

x/yScale is coming from the configuration dictionary which may be
referenced by the Graph creator, or shared with other Graphs. We need to
ensure we copy the scale so that our mutations do not change the object
given to us.

@rbu rbu Ensure Graph creates a copy of the x/yScale before modifying it
x/yScale is coming from the configuration dictionary which may be
referenced by the Graph creator, or shared with other Graphs. We need to
ensure we copy the scale so that our mutations do not change the object
given to us.
6863b96
@rbu rbu referenced this pull request Feb 5, 2014
Closed

Add xScale parameter to Graph #346

@rbu
rbu commented Feb 5, 2014

As briefly mentioned in the referenced PR that submitted the xScale config option, there's a problematic interaction between RangeSlider.Preview and HoverDetails. RangeSlider.Preview will create a new (tiny) Graph instance and configure it with the same properties as the large graph.
This way, both graphs share a reference to the same scale instance, which is modified by the RangeSlider when sliding. While this has no direct effect on the large Graph as it is already rendered, other plugins such as HoverDetails will use the Graph's scale to invert the hover coordinates to a data point.

This pull request fixes the issue by making sure a scale is never shared between Graphs.

See the before/after, where the hover dot location as well as the point determined to be nearest are both wrong.
x-scale before and after

@shushiej

Hi rbu,
I noticed from your graph on the x-axis you were able to show the dates/months, I was wondering how you were able to do this? I am not able to show dates or months on the axis, sorry this is a completely different question.

@rbu

@shushiej, I've documented this in the first post in #346, and basically works like this:

graph = new Rickshaw.Graph.Ajax({
...
    xScale: d3.time.scale(),
...
});

xAxis = new Rickshaw.Graph.Axis.X({
    graph: graph,
    tickFormat: graph.x.tickFormat()
});
@akestner

I'd be interested in getting this merged in as well, anything I can do to move things along?
Can't help but want #286 too ...

@dwt

@dchester: Could you please give a reason why you didn't merge this in yet? It seems like a simple enough bug fix for the problem we already skyped about. To reiterate, the problem is that the scale is an object as configuration instead of just being plain data and thus it can be shared with outside users. Which causes this bug here.

@dchester

I remember thinking this may be a feature in some contexts, rather than a bug. But it's obviously not doing what you want in your use case. I'll dig in and take another look here.

@dwt

Thanks! :)

@dchester dchester merged commit 897352d into shutterstock:master Apr 13, 2014
@rbu rbu deleted the dwt:clone-mutable-xscale branch Sep 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment