-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(rating): fixed model change by clicking on disabled rating element #3574
fix(rating): fixed model change by clicking on disabled rating element #3574
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3574 +/- ##
==========================================
+ Coverage 91.12% 91.71% +0.59%
==========================================
Files 95 100 +5
Lines 2793 2897 +104
Branches 516 535 +19
==========================================
+ Hits 2545 2657 +112
+ Misses 190 183 -7
+ Partials 58 57 -1
Continue to review full report at Codecov.
|
@peterblazejewicz Hello, yes |
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.
Hey, @Smoggy,
thanks for the PR. While playing with the code from the stackblitz from the issue seems to fix the problem, tests are not working:
- the one you've added passes even without the fix
- second change in
handleKeyDown
is not covered
Cheers,
Max
src/rating/rating.ts
Outdated
|
||
handleKeyDown(event: KeyboardEvent) { | ||
if (this.readonly || this.disabled) { |
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.
a807442
to
6508c16
Compare
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.
Hey, @Smoggy, thanks for fixing this.
LGTM, but could you please fix the test, as reporting is broken. You should always wrap async tests with async()
Will merge after you fix this.
6508c16
to
0a808fb
Compare
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, thanks!
Before submitting a pull request, please make sure you have at least performed the following:
Fixes #3465