Programmatically setValue is not persistent after refresh

[pimlie]

Is the slider value supposed to be always persistent after calling refresh? The docs does not say anything about this.

The problem is that when you call refresh after a setValue(), the value of the new slider element will always be reset to the initial/default value. This is because setValue only changes this._state and the input element attributes, but not this.options. And as both refresh and createNewSlider functions never check for the current state or the input element value attributes respectively, the value after a programmatical setValue is lost.

I have a PR ready that adds a boolean argument to refresh which sets the current value state as option for the newly created slider element, but I am not sure whether that is the best solution for this (eg maybe the value should be persistent by default?).

See below a test case which fails at the moment:

  it("value is persistent after refresh(true) when set programmatically", function() {
    // Initialize non-range slider
    testSlider = new Slider("#testSlider3");

    // Assert value is default
    expect(testSlider.getValue()).toBe(testSlider.defaultOptions.value);

    var sliderValue = 9
    testSlider.setValue(sliderValue)

    // Assert value has changed
    expect(testSlider.getValue()).toBe(sliderValue);

    // Invoke refresh() method
    testSlider.refresh(true);

    // Assert value is still changed
    expect(testSlider.getValue()).toBe(sliderValue);
  });

[jespirit]

You're right. refresh() will reset the value of the slider to the initial (or current) of options.value and won't persist the value after a refresh.

One workaround is to use setAttribute() along with refresh(). But you can easily forget to do that and the docs don't make it clear.

testSlider.setAttribute('value', 9);
testSlider.refresh();

To make it persistent, you would move the following code so that it executes only on creation.

https://github.com/seiyria/bootstrap-slider/blob/642b40f80790b7c82e9ff149cdf07e40907ccd34/src/js/bootstrap-slider.js#L707-L717

https://github.com/seiyria/bootstrap-slider/blob/642b40f80790b7c82e9ff149cdf07e40907ccd34/src/js/bootstrap-slider.js#L454-L457

I like the idea though. I remember I was confused when I tried refreshing a slider and it reset to its initial value.

I would recommend adding a note to the refresh() function in the documentation that lets users know the value persists.

Thoughts @rovolution ? If this was merged, would it fall under minor or patch?

[rovolution]

I think this is actually a very good idea!

From an API standpoint, I think the refresh() method should take an object of various options.

Here is my proposed API. Please leave any questions / thoughts / comments @pimlie @jespirit

// Slider refreshes with initial value (default behavior)
testSlider.refresh({
    refreshedValueType: "INITIAL" 
});

// Slider refreshes with current value
testSlider.refresh({
    refreshedValueType: "CURRENT" 
});

/*
Slider refreshes with a new value. 
Note that this call would default to the initial value if the newRefreshValue is invalid
OR
would throw a runtime error indicating that this is an invalid refresh value. 
I personally like runtime error since its more explicit but curious to hear other opinions
*/
testSlider.refresh({
    refreshedValueType: "NEW",
    newRefreshValue: 12
});

// Slider refreshes with initial value (default behavior)
testSlider.refresh();

I like passing the config on each call to refresh versus specifying on instantiation b/c it allows for more flexibility if the user wants to use different refresh types in different situations for the same slider instance.

[rovolution]

If this was merged, would it fall under minor or patch?

If calling refresh() with no arguments will lead to the current default behavior, then I would publish as a minor bump since its a new feature with no breaking changes.

If calling refresh() with no arguments will lead to a new default behavior, then I would publish as a major bump since you are breaking an existing public contract for the refresh() method.

See here for reasoning

[jespirit]

Are you saying you want to pass options object or literal into refresh() to change the slider's attributes without having to call setAttribute()? Or is it just a specific case for updating the value of the slider?

If it's the former, I prefer that rather than calling a series of setAttribute() methods and then calling refresh() to change the slider instance.

Instead of using a constant string, use a boolean (which would update options.value):

refresh({
  value: { useCurrent: true }
});

refresh({
  value: { useCurrent: false, newValue: x /* [x, y] */}
});
// OR, alternatively
refresh({
  value: { newValue: x /* [x, y] */}
});

refresh()  // default

Or use newValue option:

refresh({
  newValue: { useCurrent: true }
});

refresh({
  newValue: { useCurrent: false, value: x /* [x, y] */}
});
// OR, alternatively
refresh({
  newValue: x /* [x, y] */
});

refresh()  // default

[pimlie]

To me these solutions feel (and forgive me for this) a bit over-engineered. I like things to be as simple as possible and having to pass an object with one or more properties just feels complicated (especially using text values). Also being able to set a new, different, value seems to me to extend the scope of the refresh method outside of its expected behaviour. Refresh should either use the default (the current and behaves more like a reset method) or use the current value.

If you really want to be able to set a new value we could also just check the data type, given function refresh(newValue) we could have current behaviour with newValue === false or newValue === undefined, set the current value with newValue === true or else use newValue as the new value.

Btw, after looking at the bootstrap-slider source the name of the setAttribute method feels a bit confusing as js already has a setAttribute method for html elements. Maybe we could deprecate setAttribute in favor of setOption to have a better differentiation between the two?

E.g. deprecation could be done in a patch / minor release as follows (and then be really removed in a next major):

setAttribute: function(attribute, value) {
  console.warn('Deprecated: Calling slider.setAttribute is deprecated and will be removed in a future release. Please use slider.setOption')
  return this.setOption(attribute, value)
}

[jespirit]

To me these solutions feel (and forgive me for this) a bit over-engineered. I like things to be as simple as possible and having to pass an object with one or more properties just feels complicated (especially using text values). Also being able to set a new, different, value seems to me to extend the scope of the refresh method outside of its expected behaviour. Refresh should either use the default (the current and behaves more like a reset method) or use the current value.

Simple. I like it. (Less coding = less error-prone).

Btw, after looking at the bootstrap-slider source the name of the setAttribute method feels a bit confusing as js already has a setAttribute method for html elements. Maybe we could deprecate setAttribute in favor of setOption to have a better differentiation between the two?

E.g. deprecation could be done in a patch / minor release as follows (and then be really removed in a next major):

setAttribute: function(attribute, value) {
  console.warn('Deprecated: Calling slider.setAttribute is deprecated and will be removed in a future release. Please use slider.setOption')
  return this.setOption(attribute, value)
}

Good point. I never thought about that way. It was in the API, so I just followed the API.

[rovolution]

Refresh should either use the default (the current and behaves more like a reset method) or use the current value.

i am cool with this. However, i do think the refresh method should take an object w/ a property that specifies the desired option as a property name. a boolean or another value does not explicitly describe what is happening when refresh is invoked.

// use current
refresh({
  useCurrentValue: true
});

// use default
refresh({
  useCurrentValue: false
});

// use default
refresh();

I am also cool with refresh taking an enum that describes what is happening to the value.

// use current
refresh("USE_CURRENT_VALUE");

// use default
refresh("USE_DEFAULT_VALUE");

// use default
refresh();

Btw, after looking at the bootstrap-slider source the name of the setAttribute method feels a bit confusing as js already has a setAttribute method for html elements. Maybe we could deprecate setAttribute in favor of setOption to have a better differentiation between the two?

E.g. deprecation could be done in a patch / minor release as follows (and then be really removed in a next major):

I think we should avoid doing this if possible. If the method is documented well enough in the README, than this can be avoided. I would rather we add another example to the Examples page or add a better description.

[jespirit]

I would prefer the first syntax. The field is either true/false, which is easier to remember than typing out "USE_CURRENT_VALUE".

[pimlie]

Another question which remains is how you wish to update the value after a refresh? Should we implement something in createNewSlider to get the value from the <input>? Or do we 'just' call setValue again? Or do we change the option and set the current value as new option value?

If one of you wish to take a look at this that's fine with me too btw :)

[rovolution]

@pimlie im not too picky in terms of how the implementation is done. My initial gut instinct tells me we would just call setValue underneath the hood, but i may be wrong.

[jespirit]

The straightforward implementation is just calling setValue().

refresh: function(options) {
  this._removeSliderEventHandlers();
  createNewSlider.call(this, this.element, this.options);
  // use current value on refresh
  options = options ? options : {};
  if (options.useCurrentValue === true) {
    this.setValue(this._state.value);
  }
  if($) {
    // Bind new instance of slider to the element
    if (autoRegisterNamespace === NAMESPACE_MAIN) {
      $.data(this.element, NAMESPACE_MAIN, this);
      $.data(this.element, NAMESPACE_ALTERNATE, this);
    }
    else {
      $.data(this.element, NAMESPACE_ALTERNATE, this);
    }
  }
  return this;
},

Or alternatively, avoid having to call setValue() twice because it's already called in createNewSlider().

// add `useCurrentValue` property to `this.options`
options = options ? options : {};
this.options.useCurrentValue = options.useCurrentValue;
createNewSlider.call(this, this.element, this.options);

// Don't reset the value of the slider to its default value when calling `refresh({ useCurrentValue: true })`
if (!updateSlider || this.options.useCurrentValue !== true) {
  if (Array.isArray(this.options.value)) {
    this.options.range = true;
    this._state.value = this.options.value;
  }
  else if (this.options.range) {
    // User wants a range, but value is not an array
    this._state.value = [this.options.value, this.options.max];
  }
  else {
    this._state.value = this.options.value;
  }
}

...
this.setValue(this._state.value);

[pimlie]

@jespirit If it'd be up to me then I'd go for the second option. As I dont want to steal your thunder, are you willing to create the PR?

[jespirit]

@pimlie I don't mind really. But I'll try to get it done soon. I'll have some time in the next few days to work on this and other pull requests in the works.