Validation fix to allow for string numbers #642
Conversation
|
-1. IMO validate your data before passing it in. |
|
Sorry, some days I'm on autopilot (especially when people put what's essentially a diff in their issue). It's ultimately up to @rovolution but I think it's reasonable to require that the slider only operate on numbers. |
There was a problem hiding this comment.
Personally, I am ok with this feature (since we did say we would accept a PR for it).
See the feedback in the code for further details.
Also, could you also update the README to reflect that the setValue function takes both string and number arguments? Thanks!
| @@ -246,6 +246,14 @@ describe("Public Method Tests", function() { | |||
| expect(sliderValue).toBe(valueToSet); | |||
| }); | |||
|
|
|||
| it("properly sets the value of the slider when given a string value", function(){ | |||
There was a problem hiding this comment.
could you also add a test for when you pass a string value that cannot be properly converted into a number? This should still throw an error
For example:
testSlider.slider("setValue", "yolo");
|
@rovolution added an additional test and updated the readme. I feel as though the test is a little redundant as down in the file a little ways before the range slider tests there's a test "when an invalid value type is passed in" that passes in "a" as the value and throws and error, I can remove it if you feel the same way. |
|
@snurby7 oops just saw that test! Thanks for pointing that out! Yea for sure go ahead and delete the new test then since it would be redundant |
Pull Requests
Please accompany all pull requests with the following (where appropriate):
grunt testin your Terminal within the bootstrap-slider repository directory