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

Add multi-range support #133

Merged
merged 10 commits into from
Aug 12, 2016
Merged

Add multi-range support #133

merged 10 commits into from
Aug 12, 2016

Conversation

bayov
Copy link
Contributor

@bayov bayov commented Aug 6, 2016

Added support for more than 2 handles using <Slider range={<number>} />.
number is the number of tracks (i.e., number === nHandles + 1).

Example of <Slider range={2} value={[25, 50, 65]} />:

multi-range

CSS is different because of my application custom stylesheets...

Pushable Handles

I also need pushable handles in my webapp...

The pushable property can be set to true to enable handle-pushing. When it is a number, the number represents the minimum distance between handles when pushing (pushable={true} is equivalent to pushable={1}).

Example of <Slider range={3} pushable={10} defaultValue={[10, 25, 45, 70]} />:

pushable-handles

It also works correctly with any weird combination of marks and step.

Implementation Details

  • Instead of lowerBound and upperBound, the Slider's state was changed to store a bounds array.
  • Pushable handles calculates beforehand (and caches) all possible handle positions, taking into account marks and step. It then tries to push the surrounding handles away from the moving handle until the needed threshold (the value of pushable) is satisfied.

IMPORTANT

I couldn't manage to run the test suite. I've cloned the original slider repository. Executed:

npm install
npm start

But when I go to http://localhost:8005/examples/, I'm missing resources (they are being searched in the wrong place...). It's the same for tests.

I've only run manual tests on my webapp.

I did modify the test suite, and also added some tests for the new multi-range, but someone must run the test suite to ensure everything works...

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage decreased (-2.08%) to 56.618% when pulling 6535706 on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.08%) to 56.618% when pulling 36f1662 on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.9%) to 56.777% when pulling 169c2e7 on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage decreased (-1.9%) to 56.777% when pulling a520a56 on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage decreased (-1.9%) to 56.777% when pulling c23911f on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage decreased (-1.9%) to 56.777% when pulling 0c94d43 on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage decreased (-1.9%) to 56.777% when pulling 5d76933 on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage decreased (-1.9%) to 56.777% when pulling 734ab9d on sosz:master into 36210b3 on react-component:master.

@coveralls
Copy link

coveralls commented Aug 6, 2016

Coverage Status

Coverage decreased (-10.3%) to 48.438% when pulling b13db1e on sosz:master into 36210b3 on react-component:master.

@benjycui
Copy link
Member

benjycui commented Aug 8, 2016

It is a complicated PR, I need time to review it, thanks.

@benjycui benjycui self-assigned this Aug 8, 2016
@benjycui
Copy link
Member

👍

@benjycui benjycui merged commit 4b2f6f2 into react-component:master Aug 12, 2016
@benjycui
Copy link
Member

4.0.0

@iamricard
Copy link

Since you bumped a major version, does this have any breaking changes
or are you expecting there to be any?

@benjycui
Copy link
Member

Nope... You can consider that I make a mistake...

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.

4 participants