Allow ranges of length zero#101
Allow ranges of length zero#101Graham42 wants to merge 1 commit intodavidchin:masterfrom Graham42:zero_length_range
Conversation
|
@davidchin Thoughts on this? |
|
Hey @Graham42, thanks for your contribution. I tested it manually and it works well. However, I'd like to include this as an additive change so that it's backward compatible. Can you introduce a boolean flag, such as |
|
I thought about it and decided people actually might want a want to set a minimum length for their range. This will default to 1 for backwards compatibility, but can be set to 0 to achieve my goals. Also, I added unit tests. (Sorry should have done that the first time round) |
| disabled: false, | ||
| maxValue: 10, | ||
| minValue: 0, | ||
| minSize: 1, |
There was a problem hiding this comment.
That's a nice idea / suggestion. But I think we might have a problem with minSize set to 1 by default. This is because step value might not be 1. We want minSize to equal to step or exact multiples of step. Do you know how to resolve that problem?
There was a problem hiding this comment.
I've changed this to default to null, and use the step size if nothing is passed in. Thoughts?
davidchin
left a comment
davidchin
left a comment
There was a problem hiding this comment.
Thanks for doing the changes! I have few more comments. Let me know if you need any help.
| return new Error('Minimum range size must be a multiple of the step size'); | ||
| } | ||
| } | ||
| return PropTypes.number.apply(this, props, propName, args); |
There was a problem hiding this comment.
I think since v15.3, you're not allowed to run the validator directly. You have to call PropTypes.checkPropTypes. i.e.: PropTypes.checkPropTypes({ [propName]: PropTypes.number }, props, propName, componentName);
There was a problem hiding this comment.
looks like checkPropTypes is for checking in production. There's a paragraph just a bit further down on the page you linked that shows how we should handle this.
You might also see this error if you’re calling a PropTypes validator from your own custom PropTypes validator. In this case, the fix is to make sure that you are passing all of the arguments to the inner function. There is a more in-depth explanation of how to fix it on this page.
| import { captialize, distanceTo, isDefined, isObject, length } from '../utils'; | ||
| import { DOWN_ARROW, LEFT_ARROW, RIGHT_ARROW, UP_ARROW } from './key-codes'; | ||
|
|
||
| function minimumRangeSizeValidator(props, propName, ...args) { |
There was a problem hiding this comment.
Is it possible for you to move to a separate file, similar to range-prop-type.js?
| return ['max']; | ||
| } | ||
|
|
||
| @autobind |
There was a problem hiding this comment.
@autobind decorator is probably unnecessary in this case.
| formatLabel: PropTypes.func, | ||
| maxValue: rangePropType, | ||
| minValue: rangePropType, | ||
| minSize: minimumRangeSizeValidator, |
There was a problem hiding this comment.
I'd probably name the prop differently. Perhaps minRange might be more appropriate?
|
|
||
| @autobind | ||
| getMinRangeSize() { | ||
| return this.props.minSize !== null ? this.props.minSize : this.props.step; |
| * @param {Function} [props.formatLabel] | ||
| * @param {number|Range} [props.maxValue = 10] | ||
| * @param {number|Range} [props.minValue = 0] | ||
| * @param {number} [props.minSize = 1] // or defaults to step if step > 1 |
|
Just realized that requiring the minimum range to be a multiple of step is not a backwards compatible change. Before my original change the logic would allow a minimum range of 1 and (for example) a step of 2. So the range selected could be 1, 3, 5, etc. I'm thinking it would be best to undo the change the requires the minimum range to be a multiple of step. |
|
I can rebase and squash all these commits once you're happy with this PR |
| return values.min >= this.props.minValue && | ||
| values.max <= this.props.maxValue && | ||
| values.min < values.max; | ||
| (values.max - values.min) >= this.props.minRange; |
There was a problem hiding this comment.
I think the current solution (1 as the default minRange value) only works if step value is greater or equal to one. But it doesn't work if the step value is less than one.
I suggest making minRange optional with no default value. If minRange is not provided, use step to check against max/min values here. To ensure minRange is a multiple of step, add a check in minimumRangePropType.
| (values.max - values.min) >= this.props.minRange; | ||
| } | ||
|
|
||
| return values.max >= this.props.minValue && values.max <= this.props.maxValue; |
There was a problem hiding this comment.
We also have to make minRange prop work for single-value sliders.
|
|
||
| /** | ||
| * | ||
| * @param {*} props |
There was a problem hiding this comment.
Maybe the param type could be a little bit more specific.
Sometimes you want a range to have min / max values exactly the same. Achieve this by setting a minimum range of zero.
|
Felt like it was getting confusing. So I've made the changes and squashed everything down to 1 commit. |
|
Bump @davidchin |
|
Hey @Graham42, sorry I forgot to get back to you. I tested it earlier and found two problems:
|
|
Thanks, I'll look into it. |
|
I ended up just using this other package which suits my needs for a range input https://github.com/react-component/slider |
2 participants
Sometimes you want a range to have min / max values exactly the same.
This fixes #92, #55, and #85