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

Make hotkeys events removable #90

Open
SylvainMarty opened this issue Jun 14, 2019 · 12 comments
Open

Make hotkeys events removable #90

SylvainMarty opened this issue Jun 14, 2019 · 12 comments

Comments

@SylvainMarty
Copy link

Hi!

We just started to use this library in our VueJS project, the shortcut system works great!
But we have an important downfall with this library in our single page app, we must remove subscribed events when there are not necessary anymore.

For this, it will be very helpful to have a method destroy() to unsubscribe all the event listeners added by the library.
For now, we use the unbind() method but its not enough and the current behavior can lead to memory leaks.

We can contribute with a PR but we wanted to see if there were any cons to add this method to the library.

I'm available if there's any question. 👍

@jaywcjlove
Copy link
Owner

@SylvainMarty Oh! I haven't used Vue.js yet, you can try to solve the lead to memory leaks issue.

@bichotll
Copy link

@SylvainMarty Just create an array and keep there all the keyboard events ;)
You can then foreach the array and delete them without leaving anything behind.

@yakov116
Copy link
Contributor

@jaywcjlove thanks for the amazing library

Any update on this?

@jaywcjlove
Copy link
Owner

@SylvainMarty It can be handled in the following way.

// define shortcuts with a scope
hotkeys('ctrl+o, ctrl+alt+enter', 'issues', function(){
  console.log('do something');
});
hotkeys('o, enter', 'files', function(){ 
  console.log('do something else');
});

// set the scope (only 'all' and 'issues' shortcuts will be honored)
hotkeys.setScope('issues'); // default scope is 'all'

@jaywcjlove
Copy link
Owner

@yakov116 I think it may not be necessary. Can be achieved through scope.

@yakov116
Copy link
Contributor

@jaywcjlove can you give a bit explaining how scope works?

I read the readme and tried it in practice.
From what i understand is that you can only have one scope active at a time is that correct?

@jaywcjlove
Copy link
Owner

@yakov116 Yes, there is only one scope

@yakov116
Copy link
Contributor

@jaywcjlove ok.
Side thing do you mind if i send in a pr to improve the docs?

@jaywcjlove
Copy link
Owner

@yakov116 Welcome to improve the documentation.

@RickFrom1987
Copy link

Thanks @jaywcjlove for the awesome lib!
Bump for this issue @SylvainMarty Thx!

@SylvainMarty
Copy link
Author

@jaywcjlove Thank you for all the explanation about scopes !
Nevertheless, I don't think it will be enough to avoid event listener leakage with frameworks that use frontend routing.
@RickFrom1987 Thank you for the bump, I will definitively take some time this week to make a PR. 😉

@jaywcjlove
Copy link
Owner

@SylvainMarty 👍 Looking forward to your PR.

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

No branches or pull requests

5 participants