Skip to content

Feat/enable expressing overlap distance in step #210

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

Conversation

AmauryLiet
Copy link
Contributor

This new props allows simplifying the API from:

        allowOverlap={false}
        min={min}
        max={max}
        minMarkerOverlapDistance={
          (MIN_RANGE_LENGTH * sliderLength) / (max - min)
        }

to

        allowOverlap={false}
        min={min}
        max={max}
        minMarkerOverlapDistance={MIN_RANGE_LENGTH}

saving the implementer the need to recompute stepLength which is already used internally by the lib.

My guess being that this unit (step instead of pixels), is present more frequently in real-world use cases

@ptomasroos
Copy link
Owner

This is nice! But also a breaking change. But again I have no idea how many people who are using just this particular feature :)

@AmauryLiet
Copy link
Contributor Author

@ptomasroos it shouldn't be a breaking change as I kept the existing prop unchanged ;)

@AmauryLiet
Copy link
Contributor Author

happy to get your feedback if you have a better naming by the way

@ptomasroos
Copy link
Owner

Thanks! Missed that. Looks great, again thanks for the contribution!

@ptomasroos ptomasroos merged commit 920d716 into ptomasroos:master Sep 1, 2020
@AmauryLiet
Copy link
Contributor Author

No worries, thanks for the super quick review and merge 💪

@AmauryLiet AmauryLiet deleted the feat/enable-expressing-overlap-distance-in-step branch September 1, 2020 11:01
itenl pushed a commit to itenl/react-native-multi-slider that referenced this pull request Aug 27, 2021
* feat: Allow expressing overlap distance without re-computing stepLength

* refactor: Suggest reducing number of ternary conditions
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.

2 participants