-
-
Notifications
You must be signed in to change notification settings - Fork 999
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
test(module: slider) Add tests, Small refactors #2818
test(module: slider) Add tests, Small refactors #2818
Conversation
…double against zero to check against a tight range around zero because it will fail sometimes. Update aria attributes on handles to have proper values.
@@ -477,6 +463,7 @@ protected override void OnParametersSet() | |||
ClassMapper.Clear() | |||
.Add(PreFixCls) | |||
.If($"{PreFixCls}-disabled", () => Disabled) | |||
.If($"{PreFixCls}-horizontal", () => !Vertical) |
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.
Added this class because it is in the react version whenever it is horizontal. Didn't appear to affect any visuals for now though. Perhaps a style update will make use of it in the future and prevent a bug.
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.
Our current style is only synchronized to 4.20.7 (and many changes to the component html are not synchronized), and the latest one needs to check whether the component style is updated before merging. Could you please take a look at it?
@@ -527,23 +514,15 @@ private void ValidateParameter() | |||
throw new ArgumentOutOfRangeException(nameof(Step), "Must greater than 0."); | |||
} | |||
|
|||
if (Step != null && (Max - Min) / Step % 1 != 0) | |||
var minMaxStepComparison = (Max - Min) / Step % 1; |
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.
Is this logic necessary? I don't see it in the react library and the slider library that Ant react uses doesn't bother checking for this either. Did not doing it cause a bug before?
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.
Maybe not.
Codecov ReportBase: 40.20% // Head: 42.54% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2818 +/- ##
==========================================
+ Coverage 40.20% 42.54% +2.33%
==========================================
Files 544 544
Lines 25739 25736 -3
Branches 260 260
==========================================
+ Hits 10349 10950 +601
+ Misses 15350 14746 -604
Partials 40 40
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
LGTM
🤔 This is a ...
🔗 Related issue link
#2644
💡 Background and solution
📝 Changelog
☑️ Self Check before Merge