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
feat(timepicker): input filter to accept only numbers #3247
Conversation
@IAfanasovMob Thanks for the review. It is much appreciated here! |
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.
@fbasso Would you mind write some unit tests and/or some e2e tests for that?
de8ffd6
to
9765ded
Compare
Codecov Report
@@ Coverage Diff @@
## master #3247 +/- ##
==========================================
+ Coverage 91.12% 91.12% +<.01%
==========================================
Files 95 95
Lines 2793 2795 +2
Branches 516 516
==========================================
+ Hits 2545 2547 +2
Misses 190 190
Partials 58 58
Continue to review full report at Codecov.
|
src/timepicker/timepicker.ts
Outdated
const metaKey = e.metaKey; | ||
const shiftKey = e.shiftKey; | ||
if ([46, 8, 9, 27, 13].indexOf(keyCode) !== -1 || // Allow: Delete, Backspace, Tab, Escape, Enter | ||
(keyCode === 65 && ctrlKey === true) || // Allow: Ctrl+A |
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.
not sure if this is any better but can also do something like
([65, 67, 86, 88].indexOf(keyCode) !== -1 && (ctrlKey === true || metaKey === true)) || // Allow: Ctrl+A
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.
Thanks @msosa, yes you're right. I took this code somewhere on internet, but never had a closer look at the "if" part (it was the more complete I found, taking account of such cases). I'm going to change it.
9765ded
to
c1b0e7f
Compare
2581725
to
300348c
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.
I am fine with these changes, but questioning myself if we should land this PR or not...
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.
300348c
to
e11c77a
Compare
Done ! |
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!
@fbasso I took the liberty of refactoring / simplifying it a bit, pushed a commit. Please let me know if it is fine with you?
- Uppercase const name
-
_format(event)
→formatInput(inputElement)
Finally a better proposal than #3236 to fix #2334