search

phetsims / plinko-probability

"Plinko Probability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 7 forks source link

Replace Events with Emitter #49

Closed jessegreenberg closed 8 years ago

jessegreenberg commented 8 years ago

Sorry to add this on after the code review, but I noticed this while investigating https://github.com/phetsims/plinko-probability/issues/40.

Events.js has been marked as deprecated and should be replaced with Emitter.js. Rather than extend Emitter, Ball and Histogram should extend something else, and an Emitter should be create for each type of event that needs to be emitted.

For example, you could create an exitedEmitter, and then replace calls to trigger0 with exitedEmitter.emit().

expression-exchange has some nice emitter examples if you wish to look at them.

veillette commented 8 years ago

I had a look at expression-exchange. Ball and histogram are now extending Object rather than Events. There are no more Events in the sims.. We have now four different type of emitters.

For the balls, I added a remove listener (following a pattern I observed in expression exchange) to emitter that were attached to balls, to prevent any memory leaks.

I also commented on why this procedure is unnecessary on the histogramNode.

veillette commented 8 years ago

Assigning to @jessegreenberg for review. FYI, in the commit, there was not change in IntroModel. I must have applied the codestyle while working on this issue.

jessegreenberg commented 8 years ago

Big change @veillette, and it all looks great! Thanks.