zillow / react-slider

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

Fixing "Both slider handles on the min side..." #177

Closed geoalexidis closed 4 years ago

geoalexidis commented 4 years ago

Fixing Both slider handles on the min side when rendering slider in a dropdown https://github.com/zillow/react-slider/issues/136

This fix might also be applied to line 320, where this.resize() is beeing used, but specifically on this problem it happens during the UNSAFE_componentWillReceiveProps lifecycle

Following is a quick and dirty example to reproduce and test this behavior

Usage in a dropdown functionality

import React, { useState } from "react";
import ReactSlider from "./ReactSlider";

const TestDropdown = props => {
  const [open, setOpen] = useState(false);

  return (
    <>
      <div
        style={{ width: "100%", height: "24px", backgroundColor: "blue" }}
        onClick={() => setOpen(!open)}
      />
      <div style={{ display: open ? "block" : "none", width: "100%" }}>
        {
          <ReactSlider
            defaultValue={[15, 60]}
            max={60}
            min={5}
            renderThumb={(props, state) => (
              <div
                {...props}
                style={{
                  ...props.style,
                  backgroundColor: "#000",
                  color: "#fff",
                  padding: 10
                }}
              >
                {state.valueNow}
              </div>
            )}
            renderTrack={(props, state) => (
              <div
                {...props}
                style={{
                  ...props.style,
                  border: "20px solid #f00"
                }}
              ></div>
            )}
          />
        }
      </div>
    </>
  );
};

<TestDropdown />;
stonebk commented 4 years ago

I want to talk this through real quick.

  1. The reason the thumbs are not positioned correctly is because the slider container is display: none when the slider is first rendered, so it can't correctly calculate any sizes.

  2. When the parent container changes to display: block, UNSAFE_componentWillReceiveProps is called. At first I thought this wouldn't be the case if none of the props had changed, but since this is not a pure component, it is always called:

    Note that if a parent component causes your component to re-render, this method will be called even if props have not changed.

  3. this.resize() was being called in UNSAFE_componentWillReceiveProps, however that lifecycle event happens before the DOM is updated, so the calculation is still based on display: none.

  4. Switching to this.handleResize() works because it uses setTimeout to call this.resize() which basically defers the resize until after the DOM has updated.

While this fix does seem to work, it might be safer to move the update to componentDidUpdate to ensure that the DOM is ready to do the calculation.

stonebk commented 4 years ago

172 is similar. I think we could make a change like yours in componentDidUpdate, but also expose resize so people can call it programmatically if they need to (this was basically what people were doing by updating the key, forcing the component to re-render).

stonebk commented 4 years ago

Silly me, you can already programmatically call resize from the slider ref. Here we fix your example by calling resize after the "dropdown" is opened:

const TestDropdown = props => {
    const [open, setOpen] = React.useState(false);
    const sliderRef = React.useRef();
    React.useEffect(() => sliderRef.current.resize());

    return (
        <React.Fragment>
            <div
                style={{ width: '100%', height: '24px', backgroundColor: 'blue' }}
                onClick={() => setOpen(!open)}
            />
            <div style={{ display: open ? 'block' : 'none', width: '100%' }}>
                <ReactSlider
                    ref={sliderRef}
                    className="horizontal-slider"
                    thumbClassName="example-thumb"
                    trackClassName="example-track"
                    defaultValue={[0, 50, 100]}
                    renderThumb={(props, state) => <div {...props}>{state.valueNow}</div>}
                    pearling
                    minDistance={10}
                />
            </div>
        </React.Fragment>
    );
};

<TestDropdown />
stonebk commented 4 years ago

This is what I'm thinking: #178

geoalexidis commented 4 years ago

Hey @stonebk

Thank you very much for your analysis and your effort! 👍

Looking deeper into this issue with #178 the issue with #136 seems to be solved in a cleaner way.

I'm closing this PR then.

Have a nice day and best regards geoalexidis

stonebk commented 4 years ago

Alright, #178 is merged and published in react-slider@1.0.7!