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

possible fix for #8690 #8696

Merged
merged 2 commits into from
May 11, 2016
Merged

Conversation

designerno1
Copy link
Contributor

possible fix for #8690
assign a this var with a function.bind(this) to off only the function on _pauseEvents();
@kball
don't know if this way is best practice JS, would you please take a look

assign a this var with a function.bind(this) to off only the function on _pauseEvents()
@kball
Copy link
Contributor

kball commented Apr 28, 2016

@designerno1 thanks for pushing this forward, and for adding the visual tests. I'd prefer we break these handlers out into individual handlers per events, but this approach of binding them is good practice.

@kball
Copy link
Contributor

kball commented May 4, 2016

@designerno1 do you have time to take this PR and break the handlers into individual events so we can get this in and fix #8690?

@designerno1
Copy link
Contributor Author

@kball
really split the handlers? thats 1 more this.var for the bind and so for every on event to off we need a function
i would like to do this in vars so the _bindHandler and _bindfunctions won't be accessible from outside and don't stored on the this but i can't scope a var that's accessible from the functions, hmmmm

@kball
Copy link
Contributor

kball commented May 4, 2016

I'm open to other opinions @colin-marshall @Owlbertz but from my perspective if the logic is different for each type of event we should have a handler per event, not a joint handler with a condition. I'm more concerned with readability and maintainability than one more var on this, though if you want you could make this.handlers an object and have one per event.

@Owlbertz
Copy link
Contributor

Owlbertz commented May 5, 2016

I think the second commit with the split makes it a bit more readable, and since it's now already there I guess we should go this way.

@kball kball merged commit 106df67 into foundation:develop May 11, 2016
@designerno1 designerno1 deleted the equalizer-events branch May 11, 2016 17:36
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