-
Notifications
You must be signed in to change notification settings - Fork 105
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
Chore: Refactoring Controls for mobile #159
Conversation
src/lib/MobileControls.js
Outdated
bindControlListeners() { | ||
this.containerEl.addEventListener('touchstart', this.mousemoveHandler); | ||
this.containerEl.addEventListener('touchmove', this.mousemoveHandler); | ||
this.controlsEl.addEventListener('focusin', this.focusinHandler.bind(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.
@JustinHoldstock thoughts on a better way to do 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.
In the constructor, you'll explicitly bind the functions and reassign to themselves:
constructor() {
this.mousemoveHandler = this.mousemoveHandler.bind(this);
this.mousedownHandler = this.mousedownHandler.bind(this);
// ...
}
Then you can use the functions as usual:
this.containerEl.addEventListener('touchstart', this.mousemoveHandler);
this.containerEl.addEventListener('touchmove', this.mousemoveHandler);
this.controlsEl.addEventListener('focusin', this.focusinHandler); // Note: we remove .bind(this) here
src/lib/Controls.js
Outdated
this.containerEl.addEventListener('mousemove', this.mousemoveHandler); | ||
this.controlsEl.addEventListener('mouseenter', this.mouseenterHandler); | ||
this.controlsEl.addEventListener('mouseleave', this.mouseleaveHandler); | ||
this.controlsEl.addEventListener('focusin', this.focusinHandler.bind(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.
@JustinHoldstock also here.
@@ -47,6 +46,14 @@ class Controls { | |||
}); | |||
} | |||
|
|||
bindControlListeners() { | |||
this.containerEl.addEventListener('mousemove', this.mousemoveHandler); |
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 think we'll eventually want to move away from mouseMove/Enter/Leave
using arrow functions to the new style of binding in the constructor, but it looks good for now :)
Also, then we can remove the throttle()
from the class definition of mousemoveHandler()
to where we do the this.mousemoveHandler = throttle(this.mousemoveHandler.bind(this), THROTTLE_TIME)
* @return {void} | ||
*/ | ||
destroy() { | ||
this.containerEl.removeEventListener('touchstart', this.mousemoveHandler); |
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 above comment will apply to these
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.
Also, we might want to move to .bind()
-ing in the constructor for these guys. Your call if you wanna do it now, or later.
@tonyjin Thoughts?
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 what it'd look like (taken from a different PR):
class DashViewer extends VideoBaseViewer {
constructor() {
super();
// Bind event handlers
this.shakaErrorHandler = this.shakaErrorHandler.bind(this);
this.adaptationHandler = this.adaptationHandler.bind(this);
}
This way, we can remove autobind. This pattern follows #4 described here: https://medium.freecodecamp.com/react-binding-patterns-5-approaches-for-handling-this-92c651b5af56. We can't use #5 due to issues with arrow class properties and inheritance (React doesn't use inheritance, but composition). #1 isn't relevant. #2 has some performance implications and is also easy to forget to add the .bind. #3 has the same issues as #2 and also can't be unbound.
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.
We should not be adding .bind() unless its because of babel limitation of not being able to make super calls.
In this commit, i don't see super calls, code should remain as before
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'd rather go with a consistent approach. Having some .bind(), some = () => is super confusing, and remembering that = () => doesn't work with super (any function that could potentially be overridden cannot be = () =>)
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 its not confusing. Its very clear for whoever is familiar with ES6. Most people are familiar with that approach. Even your article above suggests that.
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.
LGTM! :D
This reverts commit 98ad9cc.
No description provided.