-
Notifications
You must be signed in to change notification settings - Fork 21
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
De20872 code review suggestion remove event listener #85
De20872 code review suggestion remove event listener #85
Conversation
client.sendEvent('userIsActive'); | ||
})); | ||
document.addEventListener('keydown', throttle(function() { | ||
var listener = throttle(function userActivityListener() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the function names here (or in the removal below). So this could just be:
var listener = throttle(function() {
client.sendEvent('userIsActive');
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lack of need is definitely true - I added them for stack-tracey things, but either way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what's the consensus - leave function name for stack-tracey-purposes, or remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good either way as well, up to you Tyler!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks. I'm good leaving the function names present. then. If we're satisfied, can you merge and version ifrau @dlockhart? Thanks :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done -- v0.17.1
.
No description provided.