Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jschen5
Copy link

@jschen5 jschen5 commented Apr 30, 2020

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
Copy link
Author

jschen5 commented Apr 30, 2020

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
Copy link
Author

jschen5 commented May 5, 2020

@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
Copy link
Owner

sghall commented May 5, 2020

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

@sghall
Copy link
Owner

sghall commented May 6, 2020

@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:

onChange(handles.map(d => d.val));

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
Copy link
Owner

sghall commented May 6, 2020

Picking this discussion up in #112

@jschen5
Copy link
Author

jschen5 commented May 6, 2020

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
Copy link
Author

jschen5 commented May 6, 2020

@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
Copy link
Author

jschen5 commented May 11, 2020

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

@jschen5
Copy link
Author

jschen5 commented May 19, 2020

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

@peancored
Copy link

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

@peancored
Copy link

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
Copy link
Author

jschen5 commented Jun 17, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants