-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
fix(Slider): value may exceed max when step is large #13632
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
base: main
Are you sure you want to change the base?
Changes from 61 commits
30af04b
37fc0b8
72c750b
f7685da
c5b0ba9
3430626
be2e46d
c7fe1a6
13248f7
42d9458
5d85c3b
8137a85
5a48f52
d24b1e7
e37c8fe
91259b9
a3774da
288cd25
b5ee53a
06b35df
22ceb87
a7e2f52
bc40733
6bbbb14
08064d3
cb5f579
736f8b4
8810099
6cfc45d
b5a0091
5ef1075
0547793
3a39f04
ee47f30
6953c40
5dd6def
ef5c686
38ce3e7
fd9c90d
b7648e0
a03d1b3
9be4cbd
ca1e404
37f1d8a
aeadef6
f9a1d0d
f8c6668
835676e
e996104
9282a87
728aae9
e8c4b50
aa4f540
46318fe
7f7ce3f
a5e8351
a90cace
572efca
d0cafc7
8de91ac
c039ba7
ac685ee
9dcc149
ffa9237
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -374,3 +374,28 @@ test('should update modelValue correctly after clicking the reversed vertical sl | |
| trigger(wrapper, 'click', 0, 100); | ||
| expect(wrapper.emitted('update:modelValue')!.pop()).toEqual([0]); | ||
| }); | ||
|
|
||
| //https://github.com/youzan/vant/issues/13625 | ||
| test('should return max when distanceToMax <= distanceToPrev', () => { | ||
| const wrapper = mount(Slider, { | ||
| props: { min: 0, max: 50, step: 60, modelValue: 45 }, | ||
| }); | ||
|
|
||
| const emitted = wrapper.emitted('update:modelValue'); | ||
| if (emitted && emitted.length > 0) { | ||
| const result = emitted[emitted.length - 1][0] as number; | ||
| expect(result).toBe(50); // Should return max | ||
| } | ||
| }); | ||
|
|
||
| test('should return prevSteppedValue when distanceToMax > distanceToPrev', () => { | ||
| const wrapper = mount(Slider, { | ||
| props: { min: 0, max: 8, step: 12, modelValue: 2 }, | ||
| }); | ||
|
|
||
| const emitted = wrapper.emitted('update:modelValue'); | ||
| if (emitted && emitted.length > 0) { | ||
| const result = emitted[emitted.length - 1][0] as number; | ||
| expect(result).toBe(0); // Should return prevSteppedValue | ||
| } | ||
| }); | ||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what circumstances would distanceToMax be greater than distanceToPrev?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
distanceToMax > distanceToPrev only occurs if prevSteppedValue is closer to value than max.
Specific conditions:
value must be between prevSteppedValue and max, and closer to prevSteppedValue.
Almost impossible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide specific examples? Because it seems like the added unit tests don't cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the added unit test cases are a bit excessive. In my opinion, it’s sufficient to add test cases for just two scenarios: distanceToMax <= distanceToPrev and distanceToMax > distanceToPrev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inottn Yes, in the description above I explained the problem of test coverage. Could you read it? #13632 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. What I mean is that the current PR adds 20 test cases, which seems excessive for a single fix. Refer to #13425 and #13008, for minor fixes, adding one or two test cases is usually sufficient. The changes involved in this fix aren't related to whether the Slider is in range or vertical mode. I think adding just 2 test cases would suffice—one to test the scenario where distanceToMax <= distanceToPrev, and another for distanceToMax > distanceToPrev.
We only need to ensure minimal test cases cover the logic of this fix, rather than increasing the test coverage of the entire component (which should be done in a separate PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inottn OK, I will modify it as you said. It will take 5 to 10 minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inottn Hi I have modified it. Many test cases I wrote earlier were to improve patch coverage. Now they have been deleted and only 2 are kept, but the patch coverage has been reduced. If you think this is appropriate!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chenjiahan @inottn Hi, do I need to change something about this?