Skip to content

feat(keycodes): add utilities for checking modifier keys - #13933

Merged
vivian-hu-zz merged 1 commit into
angular:masterfrom
crisbeto:modifier-key-utils
Nov 7, 2018
Merged

feat(keycodes): add utilities for checking modifier keys#13933
vivian-hu-zz merged 1 commit into
angular:masterfrom
crisbeto:modifier-key-utils

Conversation

@crisbeto

Copy link
Copy Markdown
Member

Based off of the conversation in #13790, these changes add some utilities for dealing with modifier keys on keyboard events.

@crisbetocrisbeto added the target: patch This PR is targeted for the next patch release label Nov 1, 2018
@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 1, 2018
@crisbeto

Copy link
Copy Markdown
MemberAuthor

@devversion I'm not too sure how the tests should be set up with Bazel since this isn't an NgModule.

@devversion

Copy link
Copy Markdown
Member

@crisbeto Have a look in the coercion entry-point. It uses the ts_library and jasmine_node_test.

@jelbourn

Copy link
Copy Markdown
Contributor

This one is definitely a feat for the next minor

@crisbeto

Copy link
Copy Markdown
MemberAuthor

@devversion by the looks of it, the jasmine_node_test isn't running in a browser environment.

@crisbetocrisbeto changed the title refactor(keycodes): add utilities for checking modifier keysfeat(keycodes): add utilities for checking modifier keysNov 2, 2018
@crisbetocrisbeto added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Nov 2, 2018
@devversion

Copy link
Copy Markdown
Member

@crisbeto Ah I didn't realize you wanted to run them in the browser. You should be able to use ts_web_test_suite (you can see an example in tools/defaults.bzl).

Just be aware that the one in defaults.bzl loads some Angular things you won't need if you just run standard tests on the browser.

@josephperrottjosephperrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@ngbot

ngbotBot commented Nov 3, 2018

Copy link
Copy Markdown

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@devversiondevversion added the in progress This issue is currently in progress label Nov 3, 2018
@crisbeto
crisbetoforce-pushed the modifier-key-utils branch 4 times, most recently from 5e4b368 to 98ad3aeCompareNovember 3, 2018 17:13
@crisbeto
crisbetoforce-pushed the modifier-key-utils branch 2 times, most recently from afc33bb to d87603aCompareNovember 3, 2018 17:17
@crisbetocrisbeto removed the in progress This issue is currently in progress label Nov 3, 2018
@crisbeto

Copy link
Copy Markdown
MemberAuthor

Sorted out the test issues with help from @devversion. @jelbourn@devversion can you take another look?

@crisbeto
crisbetoforce-pushed the modifier-key-utils branch 2 times, most recently from c1e35cc to b3d65f4CompareNovember 3, 2018 17:31
Based off of the conversation in angular#13790, these changes add some utilities for dealing with modifier keys on keyboard events.

@devversiondevversion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@devversiondevversion added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Nov 3, 2018

@jelbournjelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

* found in the LICENSE file at https://angular.io/license
*/

type ModifierKey = 'altKey' | 'shiftKey' | 'ctrlKey' | 'metaKey';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we use an enum for this?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

We could, but I wanted to keep the consumption as short as possible. With the enum people would need another import and the call would look like hasModifierKeys(event, ModifierKey.altKey, ModifierKey.shiftKey) which gets a little long.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Strings also make sense here because they are the real properties on KeyboardEvent

@vivian-hu-zz
vivian-hu-zz merged commit 7899863 into angular:masterNov 7, 2018
josephperrott pushed a commit that referenced this pull request Nov 20, 2018
Based off of the conversation in #13790, these changes add some utilities for dealing with modifier keys on keyboard events.
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 10, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreementtarget: minorThis PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@crisbeto@devversion@jelbourn@josephperrott@mmalerba@googlebot@vivian-hu-zz