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.

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

Allow to differentiate between pressing key and modifier + key #42

Closed
yarres opened this issue Dec 18, 2019 · 4 comments
Closed

Allow to differentiate between pressing key and modifier + key #42

yarres opened this issue Dec 18, 2019 · 4 comments
Assignees

Comments

@yarres
Copy link

yarres commented Dec 18, 2019

In my use case, the user should be able to create a shortcut for a specific key (e.g. 'n') and another shortcut for the same key with a shift modifier (e.g. 'shift + n').

While using the package I was not able to make it work.

Here's a simplified example where both shortcuts will be triggered at the same time when the user enters 'shift + n'. The user would only want action B to be triggered.

 ngAfterViewInit() {
    this.shortcuts.push(
      {
        key: "n",
        label: "Actions",
        description: "An action A",
        command: e => console.log("n clicked", { e }),
        preventDefault: true
      },
      {
        key: "shift + n",
        label: "Actions",
        description: "An action B",
        command: (output: ShortcutEventOutput) =>
          console.log("Shift + n clicked", output)
      }
    );
  }

I don't know if there's a way to achieve that that I didn't find or if it's impossible currently.

Anyway, thanks for this package, it's really useful :)

@yarres
Copy link
Author

yarres commented Dec 18, 2019

Since ctrl and alt are available as alternatives, it's not that big a deal if shift + n and n are not distinguished by the package. The trade offs might not be acceptable.

@omridevk
Copy link
Owner

I will look into it, AFAIK the longer sequence should win and be fire, but it I assume it depends on the order it was pressed. i.e if I pressed n then pressed shift while holding "n" I assume both events will fire, first the n, then the shift + n.
however, if shift is clicked first, I would expect that only the shift + n would fire.
Do you agree that this an acceptable behavior?
Or do you think we should wait a little to see if a combination key is pressed then if not, fire the single key event?
Please me let me know what you think.
And happy to hear you find this library helpful, hopefully I will be able to assist you with this matter as soon as possible.

@omridevk omridevk self-assigned this Dec 20, 2019
@omridevk
Copy link
Owner

In general, since there's a library that does very similar thing to what I do just not written with Rxjs or angular in mind (mousetrap), I try to keep feature parity with them and behavior parity as well, just for consistency has mousetrap has been the de-facto library for a while now for handling keyboard shortcut and I wouldn't want to diverge too much from their model.
I will check how they handle this scenario and see if I can make it behave the same.
Will keep you updated.
And again thanks for reporting the issue.

omridevk pushed a commit that referenced this issue Dec 20, 2019
…signment using 'plus' keyword; issue 40 - use event.key if possible; issue 37 - do not use rxjs internal use operator map (#43)

#42
#40
#37
@omridevk
Copy link
Owner

solved in version:
https://github.com/omridevk/ng-keyboard-shortcuts/releases/tag/8.2.0
please update and let me know if the issue persist.

ngAfterViewInit() {
    this.shortcuts.push(
      {
        key: "n",
        label: "Actions",
        description: "An action A",
        command: e => console.log("n clicked", { e }),
        preventDefault: true
      },
      {
        key: "N",
        label: "Actions",
        description: "An action B",
        command: (output: ShortcutEventOutput) =>
          console.log("Shift + n clicked", output)
      }
    );
  }


Notice the change, instead of saying shift + n, i.e capital n, you should write capital N instead.
Thanks. let me know if this solution is good enough for you.

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

No branches or pull requests

2 participants