feat(timepicker): use arrow keys to increment/decrement values #2912
Conversation
jnizet
force-pushed
the
feat/timepicker-keyboard-support
branch
from
bf15e88
to
2c9aabc
Compare
Codecov Report
@@ Coverage Diff @@
## master #2912 +/- ##
=======================================
Coverage 92.43% 92.43%
=======================================
Files 91 91
Lines 2962 2962
Branches 474 474
=======================================
Hits 2738 2738
Misses 163 163
Partials 61 61
Continue to review full report at Codecov.
|
|
Looks like its a simpler alternative to #2053 |
There was a problem hiding this comment.
Hi @jnizet!
Two comments:
- as I've commented on the previous PR fix(timepicker): fixing tab navigation & handling keyboard arrows #2053 this use case puzzles me → fix(timepicker): fixing tab navigation & handling keyboard arrows #2053 (review). Unsure how to fix this with current model update on blur.
- also I find it bizarre that cursor moves from start to end when using arrows up and down, I would expect the same behaviour as the
input type=number(MDN link), WDYT?
I think @pkozlowski-opensource meant in his comment #1334 (comment) that timepicker is broken beyond repair :) And we should decide how to redesign it (among the questions: accessibility, tab navigation, keyboard handling, input type=number, forms integration, {updateOn: blur}, etc)
Still, I think the current timepicker could still benefit from:
- adding arrow up/down support
- removing ability to focus UI arrows, so Tab would switch between inputs
|
Hi @maxokorokov Sorry I missed your comment, and also missed this previous PR. Regarding your first comment, I agree that it looks a bit weird, but it doesn't look like a deal breaker to me either (and I'm not sure how to change it either). What does look like a deal breaker is the lack of keyboard support :-) Your second comment I also agree with, but I have the feeling that fixing it would require quite a lot of dirty browser-dependant code for a small benefit. Since these two issues (IMHO) are cosmetic issues, but the lack of keyboard support is a big functional issue, can't we at least start with this and fix those issues later if necessary? |
Mousewheel support is more controversial, and would probably need an additional input + config to enable/disable it, as it's used by users which are not proficient keyboard users to scroll the page. But I think keyboard support is a must, and can be turned on always. refs ng-bootstrap#459
maxokorokov
force-pushed
the
feat/timepicker-keyboard-support
branch
from
e6048f4
to
0a39086
Compare
There was a problem hiding this comment.
LGTM, as a starting point!
Thanks @jnizet, I took the liberty to rebase.
Mousewheel support is more controversial, and would probably
need an additional input + config to enable/disable it, as it's used by
users which are not proficient keyboard users to scroll the page.
But I think keyboard support is a must, and can be turned on always.
refs #459
Before submitting a pull request, please make sure you have at least performed the following: