Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

angular/angular.js#1035 added ng-keyup and ng-keydown and tests #1622

Closed
wants to merge 3 commits into from
Closed

angular/angular.js#1035 added ng-keyup and ng-keydown and tests #1622

wants to merge 3 commits into from

Conversation

marknadig
Copy link
Contributor

I had a need for keyup as well as others described in issue #1035

@marknadig
Copy link
Contributor Author

Bah - realized I didn't update ngdoc. Let me know if I need to add that and resubmit.
Note: I signed CLA via click-through some time ago.

@pkozlowski-opensource
Copy link
Member

@digger69 Thnx for the PR! It looks like it will require some more work before it can be merged:

But, the most important thing is that I wonder if the key-up / key-down directives shouldn't support some syntax to specify on which key we want to react. Of course it could be tested with the $event that can be passed to a callback. What do you think?

@marknadig
Copy link
Contributor Author

thanks - I'll update my pull request for docs and fix the commit messages.

I don't think the directive should support which key to react to. I believe interrogating event in the handler is the correct pattern. This is consistent w/ the mouse events where similar considerations for modifiers (alt/ctrl) or buttons could be considered.

@pkozlowski-opensource
Copy link
Member

@digger69 great, thnx!

As for the directive syntax / usage I hear what you are saying. Posted a quick message on the google group to get some feedback from the community: https://groups.google.com/forum/#!topic/angular/_XZpuDRSoJ8

@marknadig
Copy link
Contributor Author

I updated the docs and fixed the commit message and tried to squash it all. Let me know how it looks. Thanks!

@IgorMinar
Copy link
Contributor

LGTM

ui-keypress is quite a beast. I don't think that it belongs to the core. we can always improve ng-keyXXX if we see that it's critical.

I personally think that it's pretty obscure (I'd rather write a custom directive than use ng-keyXXX), but since the weight of the change in bytes is minimal, I'm ok with merging this in.

@marknadig
Copy link
Contributor Author

Thanks. I like the light-weight version compared to ui-keypress as well, so looking forward to this getting in.

@ProLoser
Copy link
Contributor

Since this will not be going into the core, anyone coming across this ticket and still wants this functionality:

Go checkout AngularUI Keypress

@marknadig
Copy link
Contributor Author

I believe the intent is that this light weight directive ng-keydown will be merged into the core. The ui-keypress won't but certainly can be used independently if the extended functionality is needed. Thanks for the link ProLoser

@pkozlowski-opensource
Copy link
Member

Landed as e03182f. Thank you!

@arcanis
Copy link

arcanis commented Dec 30, 2012

Is there a reason for not adding keypress as well (I'm talking about the dom event, not the UI component) ?

If not, can I open a new pull request ?

@marknadig
Copy link
Contributor Author

@arcanis no reason, just an oversight - frankly wasn't sure what the difference was between keydown and keypress until your question prompted my search :)

@Pasvaz
Copy link

Pasvaz commented Apr 17, 2013

+1 on this pull request, since almost all the dom's events are mapped into core directives, I don't see why this one shouldn't. Keys events are usefull as the Mouse events, it makes sense to have both.

@marknadig
Copy link
Contributor Author

This ended up here: #2101 and landed f20646b

@debetimi
Copy link

Can information on these directives be added to the API docs. I think most people are unaware this feature exists. I didn't see anything about it on the angularjs.org

@marknadig
Copy link
Contributor Author

Yeah, I'm fuddled as to why it isn't showing up. I see the ngdoc reference in the source:
https://github.com/angular/angular.js/blob/master/src/ng/directive/ngEventDirs.js (see @name ng.directive:ngKeydown)
I see it on 1.1.3's docs:
http://code.angularjs.org/1.1.3/docs/api/ng.directive:ngKeydown

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants