zillow / react-slider

Accessible, CSS agnostic, slider component for React.
https://zillow.github.io/react-slider
MIT License
892 stars 231 forks source link

Dynamic slider handle size isn't accounted for #133

Closed Mike-Bell closed 5 years ago

Mike-Bell commented 6 years ago

If my slider handles are a dynamic size, they can extend beyond the limits of the slider in some situations.

Here's a concrete (and pretty reasonable / practical) example:

import * as React from 'react';
import * as Slider from './react-slider';

export default class Page1 extends React.Component<any, any>{
   constructor(){
      super();
      this.state = {sliderMax: 10, sliderValue: 5};
   }

   onSliderMove(newVal) {
      this.setState({sliderValue: newVal});
   }

   render() {
      return (
         <div>
            <div style={{width: '500px', height: '20px', border:'1px solid red'}}>
               <Slider value={this.state.sliderValue} max={this.state.sliderMax} min={1} disabled={false} onChange={this.onSliderMove.bind(this)}>
                  <div style={{border: '1px solid gray', cursor: 'pointer'}}>{`${this.state.sliderValue}/${this.state.sliderMax}`}</div>
               </Slider>
            </div>
         </div>
      )
   }
}

In this example, I display the value of the slider on the handle as it moves (a fairly reasonable thing one might want to do).

image

But once the slider ticks up to "10", the handle becomes bigger and goes off the slider. image

Playing around with the source code, one "solution" I came up with (at least for this particular example) is to always force _handleResize in componentWillReceiveProps. This could also key off of a prop, like forceResizeOnPropChange, so you could allow consumers to opt in to this less efficient behavior.

Mike-Bell commented 6 years ago

See https://github.com/mpowaga/react-slider/pull/134

mpowaga commented 6 years ago

I think that better solution would be to provide forceResize() so that users can create their own logic. The problem is that resizing the slider on each prop change is inefficient. Perhaps the best way is to use ResizeObserver but it's only supported by Chrome at the moment.

So with forceResize() method users could use the best solution for their case (probably calling that method on each prop change is fine in some situations).

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.