Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

Added support for up/down arrow keys on timepicker #2316

Closed
wants to merge 1 commit into from

Conversation

GFoley83
Copy link

@GFoley83 GFoley83 commented Jun 9, 2014

Added the ability for the user to adjust hours and minutes for the timepicker by using the up/down keyboard arrow keys. This allow the user to adjust time without the need to use the mouse as they can simply tab across the input boxes (as I haven't used e.preventDefault() for the keydown event).
By default the arrow keys are supported but this can be turned off by using an additional timepicker attribute arrow-keys="false"

Added the ability for the user to adjust hours or minutes for the timepicker by using the up/down keyboard arrow keys. This allow the user to adjust time without the need to use the mouse as they can simply tab across the input boxes (as I haven't used `e.preventDefault()`).
By default the arrow keys are supported but this can be turned off by using an additional timepicker attribute `arrow-keys="false"`
@pkozlowski-opensource
Copy link
Member

@bekos it looks OK for me on the first sight but I'm going to leave this one in your hands.

@bekos
Copy link
Contributor

bekos commented Jun 17, 2014

@GFoley83 LGTM, but I didn't understood why exactly you didn't use e.preventDefault() when you are consuming(up or down key) the event?

Also, you should add some tests in order to get this accepted.

@GFoley83
Copy link
Author

@bekos Adding e.preventDefault() at the end of the keypress event would swallow other key press e.g. tab key, which would cause accessibility issues as I wouldn't be able to move across the input fields. I could add it into each of the four if statement checks it need be? Just seemed extraneous.

I'll see if I have time to add tests later in the week.

hoursInputEl.bind('keydown', function(e) {
if ( e.which === 38 ) {
$scope.$apply($scope.incrementHours());
} else if( e.which === 40 ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix spacing

@icfantv
Copy link
Contributor

icfantv commented Oct 29, 2015

Closing in favor of #4772.

@icfantv icfantv closed this Oct 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants