BorisMoore / jsviews

Interactive data-driven views, MVVM and MVP, built on top of JsRender templates
http://www.jsviews.com/#jsviews
MIT License
855 stars 130 forks source link

jquery widgets slider range option #429

Closed johan-ohrn closed 5 years ago

johan-ohrn commented 5 years ago

The slider widget does not work with option _range=true

I made the following modification to the slider tag to make it compatible:

      slide: function(evt, ui) {
        setTimeout(function() {
          tag.updateValue(ui.values || ui.value); // property values is given when option "range" is set to true
        }, 0);

and

  setValue: function(value) {
    if (value !== undefined) {
      if ($.isArray(value))
        this.widget.values(value);
      else
        this.widget.value(value || 0);
    }
  },
johan-ohrn commented 5 years ago

I use the tag like this after the change: <div data-link="{slider range.values _range=true _min=range.min _max=range.max _orientation='horizontal'}"></div>

where the range object looks like this:

var range = {
  values: [10, 200],
  min: 10,
  max: 200
}

I noticed something which I think might be a bug? I link an input field to the values array like this <input type="text" data-link="range.values[0]" /> Dragging the slider updates the value in the textbox. However when I type a new value in the textbox I expect the slider to change but what is happening is that the value typed into the checkbox is incorrectly written to the range.values property overwriting the array instead of writing the new value to range.values[0].

BorisMoore commented 5 years ago

Thanks Johan. I'm travelling for the rest of June, so won't be able to look closer at this until returning

Thanks for working on it...

johan-ohrn commented 5 years ago

Regarding the problem of writing the value back to the array. I found this issue explicitly mentioning that it's currently not supported. Would it be possible to make it so that writing back a value to an array index (as opposed to writing back the value to an object property) triggers a refresh() of the array with the new value updated? I created this fiddle in case you need a more current demo. Just type into either of the inputs and you'll see the other one update.

johan-ohrn commented 5 years ago

I changed the setValue function into this because the jquery widget triggers change events even when it's updated with the same values. != as opposed to !== is deliberate because my values array is not only bound to the slider tag but also to input fields which will always write back a string and not a number so it seemed appropriate to skip type checking.

  setValue: function(value) {
    if (value !== undefined) {
      if ($.isArray(value)) {
        for (var i = 0; i < value.length; i++) {
          // update values on an individual basis when they change or else the slider widget will send a 'slidechange' event even if only one of the two values has changed.
          if (this.widget.values(i) != value[i]) {
              this.widget.values(i, value[i]);
          }
        }
      } else
        this.widget.value(value || 0);
    }
  },
BorisMoore commented 5 years ago

Hi Johan, thank you very much for this work.

I wonder it it might be better to have a slider API which does not require us to support binding directly to arrays of numbers or strings (issue #53), and which lets you design your data model without having to have an array for the two values which the range=true slider binds to. Instead, we can simply use:

{^{slider some.path.low some.path.high _range=true .../}}

where we bind to the first and second arguments. (Similar to the areaslider)

Here is a possible implementation:

slider: {
  bindTo: [0, 1], // Bind to first argument. If options.range=true, bind also to second argument.
  baseTag: "widget",
  widgetName: "slider",
  elem: "div",
  setSize: true,
  options: function() {
    var tag = this;
    return {
      slide: function(evt, ui) {
        if (ui.values) { // property values is given when option "range" is set to true
          tag.updateValues.apply(tag, ui.values);
        } else {
          tag.updateValue(ui.value); // property values is given when option "range" is set to true
        }
      }
    };
  },
  onAfterLink: function(tagCtx) {
    var tag = this;
    if (!tag.linkCtx.elem._jsvChg) {
      // If change not triggered by a the slider itself changing value
      tag.baseApply(arguments);
    }
  },
  setValue: function(value, index) {
    var widget = this.widget;
    if (value !== undefined) {
      value = value || 0;
      if (widget.options.range===true) {
        widget.values(index, value);
      } else {
        widget.value(value);
      }
    }
  },
  getValue: function() {
    return this.widget.value();
  }
}

Usage would be like this example:

{^{slider range.low range.high _range=true _min=range.min _max=range.max/}}

with

data.range = {low: 20, high: 150, min: 10, max: 200}

Does that address the requirements and the issues you explored?

BTW I removed the async call (setTimeout), for consistency with other analogous code in /jsviews-jqueryui-widgets.js - but there may be a case for introducing the async call. (I did some tests at some point, and ended up not using the async call, based on my tests...)

BorisMoore commented 5 years ago

I will include the above fix in the next update. Here is the "jsviews-jqueryui-widgets.js" file:

jsviews-jqueryui-widgets.js.txt

johan-ohrn commented 5 years ago

Thanks! I haven't gotten around to try it yet but I will.

BorisMoore commented 5 years ago

Range slider support added in v1.0.4 - see https://www.jsviews.com/#samples/tag-controls/jqui/slider/range