react-native-multi-slider icon indicating copy to clipboard operation
react-native-multi-slider copied to clipboard

If max prop changes dynamically, marker one would not get to the end of the track

Open richardbaltariu opened this issue 6 years ago • 8 comments

If you're going to set the max prop dynamically, probably based on a state change and you only have one marker, it would not go until the maximum value and/or position.

Test code to reproduce the issue: https://snack.expo.io/BkX6n6iV7

Steps to Reproduce

  1. Set up one Multislider component with one marker
  2. Initialize a state with two possible values (e.g.: DAYS, HOURS)
  3. Set max prop dynamically based on the above state (e.g.: max={this.state.type === 'DAYS' ? 7 : 12})
  4. Change state, check to see marker's position at maximum value

Expected Behavior

Marker should go all the way to the end of the track.

Actual Behavior

Marker stays at the value and position before the maximum one.

Testing findings

It looks like marker one calculates the maximum position based on this.state.positionTwo. By default positionTwo is undefined which is fine. When the max prop is set, it gets into componentWillReceiveProps where positionTwo gets some values regardless if it is enabled or not - which therefore, breaks the first one.

richardbaltariu avatar Jul 29 '18 23:07 richardbaltariu

First good bug report I've ever received, will try to fix this but it on be until end of August since i'm currently on vacation. Thanks for providing a example repo

ptomasroos avatar Jul 30 '18 06:07 ptomasroos

@ptomasroos Because I had to make it work as I have a short deadline, I solved it like this:

if (nextProps.values[1]) {
  var positionTwo = valueToPosition(
    nextProps.values[1],
    this.optionsArray,
    nextProps.sliderLength,
  );
  nextState.valueTwo = nextProps.values[1];
  nextState.pastTwo = positionTwo;
  nextState.positionTwo = positionTwo;
}

BUT, I would like you to implement something that you think it works best in all scenarios because you know your project better and how it would affect things.

richardbaltariu avatar Jul 30 '18 10:07 richardbaltariu

Thanks for the feedback!

ptomasroos avatar Aug 09 '18 10:08 ptomasroos

Following

nomoreboredom avatar Aug 17 '18 11:08 nomoreboredom

So what you're saying @richardbaltariu is that you think its fine for user code to send in values into the array which are larger than the user selected min, max? and if that happens it should just render to max?

I would say thats pretty easy to do within the state of the rendered of this component to not enter invalid numbers into the values section in case the min, max is constrained?

ptomasroos avatar Aug 24 '18 09:08 ptomasroos

Yes, it should be fine to render the updated min/max values. I only checked the issue for the max prop.

It would be nice to be able to set dynamic values based on state changes instead of just constrain it to what it is set when the component is first rendered, which actually works now, but it breaks for the max value as described above.

Sorry for my very late response

richardbaltariu avatar Oct 13 '18 10:10 richardbaltariu

For me I fixed it with allowOverlap={true} (even though I had only one marker).

Dror-Bar avatar Sep 08 '19 14:09 Dror-Bar

@Dror-Bar Yes, that indeed seems to fix it. Tested on the initial test code above.

Though, I still think there's a small issue in there and I think it shouldn't require that flag in order to work correctly.

richardbaltariu avatar Sep 13 '19 11:09 richardbaltariu