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

bind context to fix error #174

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

Jbcampbe
Copy link

Fix error that was missed in #171

@dgavey
Copy link
Collaborator

dgavey commented Sep 10, 2020

Thanks, I'll do a release later tonight

@dgavey dgavey merged commit 27ed574 into mharris717:master Sep 10, 2020
@boris-petrov
Copy link
Contributor

@Jbcampbe, @dgavey - I don't believe this works as intended. this.handleMouseEnter.bind(this) returns a new function every time it is called. So the call to removeEventListener won't remove the added listener because the function is different. The result from the binding in addEventListener should be saved as an instance variable or, better yet, this change to be reverted to be just this.handleMouseEnter and the handleMouseEnter to be marked as action from '@ember/object':

import { action } from '@ember/object';

...
  handleMouseEnter: action(function (e) {
    let mouseEnter = this.get('onMouseEnter');
    if (mouseEnter) {
      mouseEnter(e);
    }
  }),

@Jbcampbe
Copy link
Author

@Jbcampbe, @dgavey - I don't believe this works as intended. this.handleMouseEnter.bind(this) returns a new function every time it is called. So the call to removeEventListener won't remove the added listener because the function is different. The result from the binding in addEventListener should be saved as an instance variable or, better yet, this change to be reverted to be just this.handleMouseEnter and the handleMouseEnter to be marked as action from '@ember/object':

import { action } from '@ember/object';

...
  handleMouseEnter: action(function (e) {
    let mouseEnter = this.get('onMouseEnter');
    if (mouseEnter) {
      mouseEnter(e);
    }
  }),

Thanks for pointing out! I can get that fix in a 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

Successfully merging this pull request may close these issues.

3 participants