Use configs if they already exist on configure RangeSlider.Preview #417

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@glasnt
glasnt commented Mar 27, 2014

Fixes most of #375

When re-running configure, such as when manually changing the slider width (as per the issue), the configurations set in the initialization of the object aren't restored, but reset to scratch, only to be repopulated with the defaults.

This fix uses the same logic per Rickshaw.Graph.js:249, where this.config is only initialized as an empty object if it doesn't already exist.

@JulianRaya

The rest of the problem is in Graph.setSize, and the way it is being called in RangeSlider.Preview.
https://github.com/shutterstock/rickshaw/blob/master/src/js/Rickshaw.Graph.js#L299

If either args.width or args.height aren't defined, then that dimension is computed from the style property using window.getComputedStyle (this would probably be more accurate and would definitely be faster using this.element.offsetWidth/Height).

But in RangeSliderPreview.configure, on lines 63 and 70, you can see that the setSize method is being called twice.
https://github.com/shutterstock/rickshaw/blob/master/src/js/Rickshaw.Graph.RangeSlider.Preview.js#L63
https://github.com/shutterstock/rickshaw/blob/master/src/js/Rickshaw.Graph.RangeSlider.Preview.js#L70

  1. The first call gives width only, so setSize will try to compute the height as mentioned above.
  2. The second call gives height only, so setSize will try to compute the width as mentioned above, but since you just set the width, your setting is overwritten by the computed width.
@glasnt
glasnt commented Mar 27, 2014

So since we'll always have both a width and height argument now, either from the configuration call, or from the defaults, we can set both those elements at once, which is fine, see commit 1155048 above.

But I still see the issue per #375 (comment) where the new SVG isn't replacing the old SVG, or whatever the actual problem is.

@dchester dchester closed this in 972c2bd Apr 12, 2014
@dchester

Thanks -- this is looking better with these changes merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment