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

Child handler doesn't stopPropagation since 1.1.4 release #106

Closed
Colmea opened this issue Jun 25, 2018 · 5 comments
Closed

Child handler doesn't stopPropagation since 1.1.4 release #106

Colmea opened this issue Jun 25, 2018 · 5 comments

Comments

@Colmea
Copy link

Colmea commented Jun 25, 2018

Hello,

First thank you for your package.

Since the 1.1.4 release, and especially this Pull Request (https://github.com/greena13/react-hotkeys/pull/101/files), there is a breaking change regarding event propagation to parent's handler.

Before this release, a hotkey defined in a child component AND in a root component was only triggered once (by the most nested component). But now, they are always both triggered (nested component AND root component).

I guess this happens because we create a new handler on each render() (and so hasChanged(handlers, prevHandlers) is always true) (see: https://github.com/greena13/react-hotkeys/pull/101/files#diff-5f147d6760a1bc2314f409768a38984fR247).

It means that your example in the README won't stop propagation to parent because a new handler is created on each render:

const MyNode = React.createClass({
  render() {
    const handlers = {
      'deleteNode': this.deleteNode
    };

    return (
      <HotKeys handlers={handlers}>
        Node contents
      </HotKeys>
    );
  }
});

Is it a (new) expected behavior ? If its is, shouldn't we mark this PR as a breaking change ?

We had to rollback to previous release to fix this issue, and I guess we're not the only one to create a new handler on each render (bad idea though, but a breaking change nevertheless)

@greena13
Copy link
Owner

Hello Colmea, thank you for your patience on this.

Can you please confirm the latest version of the package that still works for you? The hasChanged() logic appears to have been in since v1.1.1.

@greena13
Copy link
Owner

Are you are to provide any more specifics on this, Colmea?

@natew
Copy link
Contributor

natew commented Oct 24, 2018

I'm seeing this as well, anyone have a workaround? I can submit a PR, is this still maintained? I love the API but afraid this has been open so long it's a dead project.

@Colmea
Copy link
Author

Colmea commented Oct 25, 2018

Hey @greena13 , sorry for the delay.
Indeed, the 1.0.6 is the last working release for us.

@greena13
Copy link
Owner

This will be resolved in v2.0.0-pre1 when it's released soon.

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

3 participants