sghall / react-compound-slider

:black_medium_small_square: React Compound Slider | A small React slider with no opinion on markup or styles
https://react-compound-slider.netlify.com
MIT License
626 stars 80 forks source link

call onSlideStart from handleRailAndTrackClicks #113

Open jschen5 opened 4 years ago

jschen5 commented 4 years ago

I've basically replicated the logic from onStart where we call onSlideStart with all of the handles and the active handle's id and placed it in handleRailAndTrackClicks.

jschen5 commented 4 years ago

I didn't see any tests for the functionality within handleRailAndTrackClicks, but it's very possible that I missed them when I read through the tests. Let me know if I should add tests somewhere for the new functionality.

jschen5 commented 4 years ago

@sghall just bumping this PR. Whenever you get a chance, could you take a quick look? The changes are pretty minimal, at least as I understand the issue, and our implementation relies on knowing when dragging starts, even if it's not from directly clicking a handle.

sghall commented 4 years ago

Hey @jschen5 thanks for bumping this. Yep, missed it. I'll take a look shortly.

sghall commented 4 years ago

@jschen5 I can't merge this as is. This is being called with the "candidate" handle values before they are finalized. This would definitely break other people's code. Thing is, the onChange callback is called right after this with the handles so I think you could pick up the values there.

That's here in the code: https://github.com/sghall/react-compound-slider/blob/74e702059af6ed53280479610bd55c26fa6c019b/src/Slider/Slider.tsx#L485

If that doesn't work for you, maybe you could explain the underlying issue and fork one of the sandboxes to demo it. A bunch of sandboxes here: https://github.com/sghall/react-compound-slider#more-examples-on-codesandbox

sghall commented 4 years ago

Picking this discussion up in https://github.com/sghall/react-compound-slider/issues/112

jschen5 commented 4 years ago

here's a forked sandbox where it's now working - it just console logs when clicking to start a slide https://codesandbox.io/s/react-compound-slider-20knb

jschen5 commented 4 years ago

@sghall just updated the pr. I wasn't sure if adding the flag param was the best approach, but I needed a way to tell submitUpdate to call onSlideStart only for that specific situation.

jschen5 commented 4 years ago

Hi @sghall! Just bumping this again since it's been a few days. Thanks!

jschen5 commented 4 years ago

Hi @sghall, figured I bump this pr one last time, otherwise I'll just find a workaround. Thanks

tonygoldcrest commented 4 years ago

Hi @jschen5, I stumbled upon the same issue. Have you found a workaround?

tonygoldcrest commented 4 years ago

I have found a workaround. For anyone who may face the same issue, the solution that worked for me is:

  1. Pass the onSlideStart callback to the custom Rail component.
  2. Bind onMouseDown to onSlideStart:
    {...getRailProps({
    onMouseEnter: onMouseEnter,
    onMouseLeave: onMouseLeave,
    onMouseDown: onSlideStart
    })}

    Not sure whether there are any issues with this approach, but it seems to have worked in my case.

jschen5 commented 4 years ago

Sorry, just saw this pr was still active 🙂 That's exactly the workaround we went with. Ideally, the behavior would be built-in to the library, but not sure when @sghall will take a look at this again.