-
-
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
Open
lllomh
wants to merge
63
commits into
youzan:main
Choose a base branch
from
lllomh:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
63 commits
Select commit
Hold shift + click to select a range
30af04b
fix(Picker): fix bug the content of the popup window is not consiste…
lllomh 37fc0b8
fix(Picker): Fix the issue that scroll wheel selection list is not su…
lllomh 72c750b
fix(Picker): Rollback of new commits last one
lllomh f7685da
Merge branch 'main' into main
lllomh c5b0ba9
Merge branch 'youzan:main' into main
lllomh 3430626
Merge branch 'youzan:main' into main
lllomh be2e46d
fix(Button): When the step value of the Slider component is relativel…
lllomh c7fe1a6
fix(Button): When the step value of the Slider component is relativel…
lllomh 13248f7
fix(Button): When the step value of the Slider component is relativel…
lllomh 42d9458
fix(Button): When the step value of the Slider component is relativel…
lllomh 5d85c3b
fix(Button): When the step value of the Slider component is relativel…
lllomh 8137a85
fix(Button): When the step value of the Slider component is relativel…
lllomh 5a48f52
fix(Button): When the step value of the Slider component is relativel…
lllomh d24b1e7
fix(Button): When the step value of the Slider component is relativel…
lllomh e37c8fe
fix(Button): When the step value of the Slider component is relativel…
lllomh 91259b9
fix(Button): When the step value of the Slider component is relativel…
lllomh a3774da
fix(Button): When the step value of the Slider component is relativel…
lllomh 288cd25
fix(Button): When the step value of the Slider component is relativel…
lllomh b5ee53a
fix(Button): When the step value of the Slider component is relativel…
lllomh 06b35df
fix(Button): When the step value of the Slider component is relativel…
lllomh 22ceb87
fix(Button): When the step value of the Slider component is relativel…
lllomh a7e2f52
fix(Button): When the step value of the Slider component is relativel…
lllomh bc40733
fix(Button): When the step value of the Slider component is relativel…
lllomh 6bbbb14
fix(Button): When the step value of the Slider component is relativel…
lllomh 08064d3
fix(Button): When the step value of the Slider component is relativel…
lllomh cb5f579
fix(Button): When the step value of the Slider component is relativel…
lllomh 736f8b4
fix(Button): When the step value of the Slider component is relativel…
lllomh 8810099
fix(Button): When the step value of the Slider component is relativel…
lllomh 6cfc45d
fix(Button): When the step value of the Slider component is relativel…
lllomh b5a0091
fix(Button): When the step value of the Slider component is relativel…
lllomh 5ef1075
fix(Button): When the step value of the Slider component is relativel…
lllomh 0547793
fix(Button): When the step value of the Slider component is relativel…
lllomh 3a39f04
fix(Button): When the step value of the Slider component is relativel…
lllomh ee47f30
fix(Button): When the step value of the Slider component is relativel…
lllomh 6953c40
fix(Button): When the step value of the Slider component is relativel…
lllomh 5dd6def
fix(Slider): When the step value of the Slider component is relativel…
lllomh ef5c686
fix(Slider): When the step value of the Slider component is relativel…
lllomh 38ce3e7
fix(Slider): When the step value of the Slider component is relativel…
lllomh fd9c90d
fix(Slider): When the step value of the Slider component is relativel…
lllomh b7648e0
fix(Slider): When the step value of the Slider component is relativel…
lllomh a03d1b3
fix(Slider): When the step value of the Slider component is relativel…
lllomh 9be4cbd
fix(Slider): When the step value of the Slider component is relativel…
lllomh ca1e404
fix(Slider): When the step value of the Slider component is relativel…
lllomh 37f1d8a
fix(Slider): When the step value of the Slider component is relativel…
lllomh aeadef6
fix(Slider): When the step value of the Slider component is relativel…
lllomh f9a1d0d
fix(Slider): When the step value of the Slider component is relativel…
lllomh f8c6668
fix(Slider): When the step value of the Slider component is relativel…
lllomh 835676e
fix(Slider): When the step value of the Slider component is relativel…
lllomh e996104
fix(Slider): When the step value of the Slider component is relativel…
lllomh 9282a87
fix(Slider): When the step value of the Slider component is relativel…
lllomh 728aae9
fix(Slider): When the step value of the Slider component is relativel…
lllomh e8c4b50
fix(Slider): When the step value of the Slider component is relativel…
lllomh aa4f540
fix(Slider): When the step value of the Slider component is relativel…
lllomh 46318fe
fix(Slider): When the step value of the Slider component is relativel…
lllomh 7f7ce3f
fix(Slider): When the step value of the Slider component is relativel…
lllomh a5e8351
fix(Slider): When the step value of the Slider component is relativel…
lllomh a90cace
fix(Slider): When the step value of the Slider component is relativel…
lllomh 572efca
Merge branch 'main' into main
lllomh d0cafc7
fix(Slider): When the step value of the Slider component is relativel…
lllomh 8de91ac
fix(Slider): When the step value of the Slider component is relativel…
lllomh c039ba7
fix(Slider): When the step value of the Slider component is relativel…
lllomh ac685ee
fix(Slider): When the step value of the Slider component is relativel…
lllomh 9dcc149
fix(Slider): When the step value of the Slider component is relativel…
lllomh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?