Skip to content

Feature/restricted range #583

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

Closed

Conversation

DanielReid
Copy link
Contributor

Hi, we added a restricted range because we needed one, and this slider seemed the best starting point.

@codecov-io
Copy link

codecov-io commented Oct 24, 2017

Codecov Report

Merging #583 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #583   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         950    970   +20     
=====================================
+ Hits          950    970   +20
Impacted Files Coverage Δ
src/rzslider.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1e69b0...53ddb8e. Read the comment docs.

@ValentinH
Copy link
Member

Hi, thanks for this pull request!
Could you provide a demo (via JSFiddle for example) that show the new feature?

@DanielReid
Copy link
Contributor Author

We updated the demo with a restricted range slider as well, so checkout should also work.

Here's a fiddle: https://jsfiddle.net/2hxm9cop/1/

@ValentinH
Copy link
Member

This feature is really nice, well done!

I just have one small issue to raise:
slider-restriction

In the above example, when I switch from 30 to 70, the slider handle only moves to 70 when I'm exactly at 70. A better behavior would be to move to 70 when the mouse is just after the middle of the restricted range (51). What do you think?

We already have the same behavior for large steps...

@DanielReid
Copy link
Contributor Author

Thanks for the compliment (and the work maintaining this project!). Cool gif! That's a clever way to communicate these things.

For us this is intended behaviour because in our use case we wish to discourage moving from one side to the other. It could be an option, like restrictedRange.switchSidesAtMiddle perhaps?

@ValentinH
Copy link
Member

For the GIF, I use this really nice tool: https://getkap.co

About the option, I don't really know how to approach this. I think the middle thing should be the default and we should add an option to address your use case. Do you have an idea of how we could design this option?

@DanielReid
Copy link
Contributor Author

I've decided it's best to not overdesign this, and have changed it to switch in the middle instead. Extra checks can be done by consumers of the slider's model values anyway.

@DanielReid
Copy link
Contributor Author

@ValentinH is there anything else you would like changed about this? We're currently thinking about allowing more than one restricted range, but that's probably best as a separate PR once this one's complete.

@ValentinH
Copy link
Member

Hey,

It's been a long time! ^^ Just tried your last fiddle and it looks fine. Could you rebase your changes on top of master so I can review it?

Thanks by advance!

@DanielReid DanielReid closed this Jun 29, 2018
@DanielReid DanielReid deleted the feature/restrictedRange branch June 29, 2018 13:06
@DanielReid
Copy link
Contributor Author

Hi, I made a new branch/PR as per request (#638)

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.

3 participants