search

phetsims / area-builder

"Area Builder" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/area-builder
GNU General Public License v3.0
1 stars 2 forks source link

problems with AreaAndPerimeterDisplay #114

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Noted during conversion of AreaAndPerimeterDisplay.js to ES6 class for https://github.com/phetsims/tasks/issues/1044.

Three problems with AreaAndPerimeterDisplay:

(1) It defines this.expandedProperty, which is also defined in the superclass constructor

(2) It overrides reset with a method that is identical to the superclass reset.

(3) It passes options to the superclass constructor and calls this.mutate. This is an anti-pattern that I'm running into frequently - maybe we should discuss?

pixelzoom commented 4 years ago

Problems were addressed in the above commits.

58bee29 does ES6 class conversion and deletes this.expandedProperty and reset. Sorry they had to be done in the same commit, but you were using this before calling super. I confirmed the Reset All is working properly.

e5a9de3 deletes this.mutate and passes all options via the superclass constructor.

@jbphet please review.

jbphet commented 4 years ago

The changes look good and I tested the behavior and it looks correct. Thanks @pixelzoom. Closing.