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

Clicking the track does not trigger onAfterChange #180

Closed
brandonaaron opened this issue May 20, 2020 · 3 comments · Fixed by #186
Closed

Clicking the track does not trigger onAfterChange #180

brandonaaron opened this issue May 20, 2020 · 3 comments · Fixed by #186

Comments

@brandonaaron
Copy link

Description

Clicking directly on the track will trigger an onChange event but does not trigger onBeforeChange nor onAfterChange.

CodeSandbox

This can be seen on the docs but also reproduced in the code sandbox.

Edit zillow/react-slider

@viters
Copy link
Contributor

viters commented May 23, 2020

I also would love onAfterChange to be triggered after every change. onChange triggers too often for some controlled-scenarios in which only the end result matters, where onAfterChange would be perfect. Although, now onAfterChange is rather useless, because it does not guarantee to capture all user interactions.

@viters
Copy link
Contributor

viters commented May 24, 2020

At the same time, onSliderClick returns only one value even if slider contains multiple handles. It is less useful than receiving new value, because it is hard to tell which handle did change.

There is a workaround though @brandonaaron , you can bind to onSliderClick and use ReactSlider.getValue.

@stonebk
I think one of these scenarios would be worth considering:

  • fire onAfterChange right after onChange at onSliderMouseDown - so adding onAfterChange to slider track click, which is breaking change
  • return whole value instead of only one changed part from onSliderClick - it is also breaking change, but I cannot think of any use scenario of onSliderClick right now
  • return whole value as second parameter of onSliderClick - it is not a breaking change, but it's kinda tech debt in my opinion

@brandonaaron
Copy link
Author

Thanks for the workaround idea. I ended up using that for now.

Passing the whole value as the second parameter of onSliderClick makes the most sense to me with the priority of reducing breaking changes. I agree it is tech debt but probably better to take that on, rather than deal with breaking stuff.

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 a pull request may close this issue.

2 participants