glenjamin / react-hotkey

A simple mixin for application hotkeys
MIT License
35 stars 12 forks source link

hotkey.deactivate or similar? #9

Open t1mmen opened 8 years ago

t1mmen commented 8 years ago

Hi,

First off, thanks for a great mixin! I'm looking manual hooks similar to what componentDidMount and componentWillUnmount already do, allowing this sort of functionality:

   componentWillReceiveProps(nextProps) {
     if (nextProps.isHotkeyActive) {
       hotkey.activate();
     } else {
       hotkey.deactivate(); // hotkey.disable unregisters all listeners :/
     }
   },

Is this something you're planning on adding support for?

glenjamin commented 8 years ago

Is there anything in that example that doesn't work? It looks like it should be fine.

To add this to core would be another mixin, and I'm not sure it's a particularly common use-case - and presumably you can only really do that once per app anyway.

For that reason I don't think it's worth adding to the core lib

t1mmen commented 8 years ago

hotkey.deactivate() isn't a method, and hotkey.disable() unregisters all listeners for that event key, I was just trying to illustrate what I wanted to do.

Thanks though, I'll figure out a solution locally. Let me know if you change your mind re: core lib, and I'll submit a PR when I get around to fixing this issue.

glenjamin commented 8 years ago

Oh, I see - you want to disable it per-component, not globally?

glenjamin commented 8 years ago

@t1mmen One option would be to return; in the event handler is the isActive prop is not set.

t1mmen commented 8 years ago

@glenjamin Correct, I'd like to control it per component. I'll try return too, although I was pretty sure I gave that a shot already.

t1mmen commented 8 years ago

What I ended up doing, was creating a simple wrapper component and pass along isActive to determine if it keyup should propagate. I'm unsure what (if any) performance impact that might have compared to my original suggestion (since the even handlers are still bound even when isActive is falsey)

// Usage 
<HotkeyTrap isActive={this.state.isActive} key="Escape" onTrigger={this.handleTogglePopover}>
  {/* Whatever */}
</HotkeyTrap>
// Component
import React from 'react';

const hotkey = require('react-hotkey');
hotkey.activate();

export default React.createClass({
  mixins: [ hotkey.Mixin('handleHotkey')],

  getDefaultProps() {
    return {
      key: 'Escape'
    };
  },

  handleHotkey: function(e) {
    if (!this.props.isActive) return;

    if (e.key === this.props.key && this.props.onTrigger) {
      this.props.onTrigger(e);
    }

  },

  render() {
    return (
      <div className={this.props.className}>
        {this.props.children}
      </div>
    );
  }
});

Feel free to close this issue.

glenjamin commented 8 years ago

That local good to me - perf Impact should be minimal because there's only ever one handler registered.