fix: check for invalid configuration in limit settings #638
Conversation
There was a problem hiding this comment.
If lowerLimit > upperLimit we shouldn't fallback to upperLimit. If we do that, then we will end up with having lowerLimit === upperLimit, which will prevent Slider from moving at all. This is not the best experience.
If a dev/user makes a simple mistake, typo, of having lowerLimit greater than the upper one, then we should just avoid setting that incorrect limit. In that worst case, the limit that is incorrect will just not work, not the whole Slider.
package/ios/RNCSliderManager.m
Outdated
| if (upperLimit < view.lowerLimit) { | ||
| upperLimit = view.lowerLimit; | ||
| NSLog(@"Invalid configuration: reverting lower limit to equal to upper limit") | ||
| } | ||
|
|
||
| view.upperLimit = upperLimit; |
There was a problem hiding this comment.
| if (upperLimit < view.lowerLimit) { | |
| upperLimit = view.lowerLimit; | |
| NSLog(@"Invalid configuration: reverting lower limit to equal to upper limit") | |
| } | |
| view.upperLimit = upperLimit; | |
| if (upperLimit < view.lowerLimit) { | |
| NSLog(@"Invalid configuration: upperLimit < lowerLimit; upperLimit not set") | |
| } else { | |
| view.upperLimit = upperLimit; | |
| } |
package/ios/RNCSliderManager.m
Outdated
| if (lowerLimit > view.upperLimit) { | ||
| lowerLimit = view.upperLimit; | ||
| NSLog(@"Invalid configuration: reverting upper limit to equal to lower limit") | ||
| } | ||
|
|
||
| view.lowerLimit = lowerLimit; |
There was a problem hiding this comment.
| if (lowerLimit > view.upperLimit) { | |
| lowerLimit = view.upperLimit; | |
| NSLog(@"Invalid configuration: reverting upper limit to equal to lower limit") | |
| } | |
| view.lowerLimit = lowerLimit; | |
| if (upperLimit < view.lowerLimit) { | |
| NSLog(@"Invalid configuration: upperLimit < lowerLimit; lowerLimit not set") | |
| } else { | |
| view.lowerLimit = lowerLimit; | |
| } |
package/src/Slider.tsx
Outdated
| }, [lowerLimit, upperLimit]); | ||
|
|
||
| useEffect(() => { | ||
| limitCheck(); |
There was a problem hiding this comment.
We don't need to create separate callback for limitCheck and use it only in the effect.
Let's have that single if check and console.warn directly in this effect.
Also, if the callback uses lowerLimit, upperLimit for dependencies then I don't see a reason for using anything different in the dependencies of this effect.
| mLowerLimit = (int) Math.round((limit - mMinValue) / (mMaxValue - mMinValue) * getTotalSteps()); | ||
| int lowerLimit = (int) Math.round((limit - mMinValue) / (mMaxValue - mMinValue) * getTotalSteps()); | ||
| if(lowerLimit > mUpperLimit) { | ||
| Log.w("Invalid configuration", "reverting lower limit to upper limit"); |
There was a problem hiding this comment.
We shouldn't display logs in very different format for the same type of error.
Let's unify all of them to the one proposed in the suggestions for iOS platform (of course considering whether it's for upperLimit or lowerLimit).
| if(mLowerLimit > upperLimit) { | ||
| Log.w("Invalid configuration", "reverting upper limit to equal to lower limit"); | ||
| } | ||
| mUpperLimit = Math.max(upperLimit, mLowerLimit); |
There was a problem hiding this comment.
| if(mLowerLimit > upperLimit) { | |
| Log.w("Invalid configuration", "reverting upper limit to equal to lower limit"); | |
| } | |
| mUpperLimit = Math.max(upperLimit, mLowerLimit); | |
| if (mLowerLimit > upperLimit) { | |
| Log.w("Invalid configuration", "upperLimit < lowerLimit; upperLimit not set"); | |
| } else { | |
| mUpperLimit = upperLimit; | |
| } |
Summary:
fixes: #571
Apply additional check and move the progress calculation to Slider class
Test Plan:
Have ran the application examples on new/old arch witch success - the crash is not happening anymore.