-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
call onSlideStart from handleRailAndTrackClicks #113
base: master
Are you sure you want to change the base?
Conversation
I didn't see any tests for the functionality within |
@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. |
Hey @jschen5 thanks for bumping this. Yep, missed it. I'll take a look shortly. |
@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 That's here in the code: react-compound-slider/src/Slider/Slider.tsx Line 485 in 74e7020
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: |
Picking this discussion up in #112 |
here's a forked sandbox where it's now working - it just console logs when clicking to start a slide |
@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 |
Hi @sghall! Just bumping this again since it's been a few days. Thanks! |
Hi @sghall, figured I bump this pr one last time, otherwise I'll just find a workaround. Thanks |
Hi @jschen5, I stumbled upon the same issue. Have you found a workaround? |
I have found a workaround. For anyone who may face the same issue, the solution that worked for me is:
{...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. |
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. |
I've basically replicated the logic from
onStart
where we callonSlideStart
with all of the handles and the active handle's id and placed it inhandleRailAndTrackClicks
.