Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 7, 2018
Merged

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

merged 1 commit into from
Nov 7, 2018

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Nov 1, 2018

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

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

crisbeto commented Nov 1, 2018

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

@devversion
Copy link
Member

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

@jelbourn
Copy link
Member

jelbourn commented Nov 1, 2018

This one is definitely a feat for the next minor

@crisbeto
Copy link
Member Author

crisbeto commented Nov 2, 2018

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

@crisbeto crisbeto changed the title refactor(keycodes): add utilities for checking modifier keys feat(keycodes): add utilities for checking modifier keys Nov 2, 2018
@crisbeto crisbeto 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
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.

josephperrott
josephperrott approved these changes Nov 2, 2018
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@ngbot
Copy link

ngbot bot commented Nov 3, 2018

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

@devversion devversion added the in progress This issue is currently in progress label Nov 3, 2018
@crisbeto crisbeto force-pushed the modifier-key-utils branch 4 times, most recently from 5e4b368 to 98ad3ae Compare November 3, 2018 17:13
@crisbeto crisbeto requested a review from devversion as a code owner November 3, 2018 17:13
@crisbeto crisbeto force-pushed the modifier-key-utils branch 2 times, most recently from afc33bb to d87603a Compare November 3, 2018 17:17
@crisbeto crisbeto removed the in progress This issue is currently in progress label Nov 3, 2018
@crisbeto
Copy link
Member Author

crisbeto commented Nov 3, 2018

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

@crisbeto crisbeto force-pushed the modifier-key-utils branch 2 times, most recently from c1e35cc to b3d65f4 Compare November 3, 2018 17:31
@crisbeto
feat(keycodes): add utilities for checking modifier keys
3e4ea4e 
Based off of the conversation in angular#13790, these changes add some utilities for dealing with modifier keys on keyboard events.
devversion
devversion approved these changes Nov 3, 2018
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM.

@devversion devversion added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Nov 3, 2018
jelbourn
jelbourn approved these changes Nov 5, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

mmalerba
mmalerba reviewed Nov 6, 2018
View reviewed changes
src/cdk/keycodes/modifiers.ts
* found in the LICENSE file at https://angular.io/license
*/

type ModifierKey = 'altKey' | 'shiftKey' | 'ctrlKey' | 'metaKey';
Copy link
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
Member Author

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
Member

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:master Nov 7, 2018
josephperrott pushed a commit that referenced this pull request Nov 20, 2018
@crisbeto @josephperrott
feat(keycodes): add utilities for checking modifier keys (#13933)
41f1c9d 
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

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-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba mmalerba left review comments

@devversion devversion devversion approved these changes

@josephperrott josephperrott josephperrott approved these changes

Assignees

@jelbourn jelbourn

@devversion devversion

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@crisbeto @devversion @jelbourn @josephperrott @mmalerba @googlebot @vivian-hu-zz