-
Notifications
You must be signed in to change notification settings - Fork 179
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
Event logging update #212
Event logging update #212
Conversation
//pass invalid event | ||
if (!keyEvent.key) return; | ||
// don't send events for keys that don't change the input | ||
if (keyEvent.metaKey || [9, 27, 37, 39, 13, 38, 40].indexOf(keyEvent.keyCode) !== -1) return; |
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.
Can you add a comment with the actual keys that these codes correspond to
lib/events.js
Outdated
if (err) return this.handleError(err, callback); | ||
if (res.statusCode != 204) return this.handleError(res, callback); | ||
if (err) return _this.handleError(err, callback); | ||
if (res.statusCode != 204) return _this.handleError(res, callback); | ||
if (callback) return callback(); |
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.
This is duplicative of line 104 in that callback
is populated if it is falsy. Line 104 should be removed.
lib/events.js
Outdated
if (err) return this.handleError(err, callback); | ||
if (res.statusCode != 204) return this.handleError(res, callback); | ||
if (err) return _this.handleError(err, callback); | ||
if (res.statusCode != 204) return _this.handleError(res, callback); |
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.
Are we able to use arrow functions? If possible let's switch this to an arrow function to avoid the rescope of this
and avoid having to use _this
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.
No arrow functions here just now -- perhaps in a future sprint we can bring support for arrow functions.
In lieu of arrow functions, however, I added a bind(this)
so we don't need to create a temporary _this
variable.
* @param {Function} callback the function to execute after the events have been sent successfully | ||
* @private | ||
*/ | ||
flush: function(){ |
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.
The JS Doc says this function should have a callback
param
🌮 |
This PR updates the interaction logging first introduced in #183 in the following ways:
place_name
andid
are included with selection events so that these events can be more easily consumed by search engine updatesa new
keyevent
event is introduced. This event will be sent on each keydown event -- these anonymous events will be used to further improve relevance computation in the core search engine.events are batched together to minimize network calls
briefly describe the changes in this PR
write tests for all new functionality
update CHANGELOG.md with changes under
master
heading before merging