search

phetsims / curve-fitting

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

fitProperty should be an instance of EnumerationProperty #149

Closed pixelzoom closed 5 years ago

pixelzoom commented 5 years ago

Related to #143 code review.

In CurveFittingModel, this should be an EnumerationProperty:

      // @public {Property.<FitType>}, the method of fitting the curve to data points
      this.fitProperty = new Property( FitType.BEST, { validValues: FitType.VALUES } );

I.e.:

      // @public the method of fitting the curve to data points
      this.fitProperty = new EnumerationProperty( FitType, FitType.BEST );
SaurabhTotey commented 5 years ago

Sounds good, I'll get on that. Perhaps the documentation here https://github.com/phetsims/phet-core/blob/master/js/Enumeration.js#L40-L44 should be updated as well then.

SaurabhTotey commented 5 years ago

I have made the change for CurveFittingModel. Assigning to @pixelzoom to review.

pixelzoom commented 5 years ago

👍 Commit looks good.

I don't think the documentation for Enumeration should say anything about EnumerationProperty, for the same reason that Vector2 shouldn't say anything about Vector2Property. Those classes are values, and their API and doc shouldn't know anything about Property.