phetsims / density

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

`massNumberProperty` and `numberControlVolumeProperty` need units and consistent names #155

Closed arouinfar closed 1 year ago

arouinfar commented 1 year ago

For #150

On the Intro Screen, the Mass NumberControl has an internal massNumberProperty. I decided to unfeature it in #137 because It is identical to the massProperty found in the model. The only usage of massNumberProperty seems to be a dependency for the numberDisplay.valueText.

image

I don't think massNumberProperty or massNumberControl.numberDisplay.valueText add any real value. We use the same mass units in the model and the view. Can these be uninstrumented @jonathanolson?

Similarly, the Volume NumberControl has an associated numberControlVolumeProperty which appears to be the volume in liters, though its units are null. The only usage of numberControlVolumeProperty that I can find is a dependency of the associated numberDisplay.valueText. However, this seems like an odd way to give clients access to the volume in liters. The method in density.compareScreen.view.volumeNumberControl.numberDisplay.valueText.stringProperty is preferable.

@jonathanolson can we instrument numberControlVolumeProperty and use the Compare Screen method to display the NumberDisplay contents?

jonathanolson commented 1 year ago

Added units to the intermediate Properties above.

arouinfar commented 1 year ago

Discussed with @jonathanolson and @DianaTavares

The intermediary properties are necessary to the internal workings of the sim, so this is a big ask. Instead, we decided to consistently name these properties, numberControlMassProperty and numberControlVolumeProperty and @jonathanolson has already added units.

jonathanolson commented 1 year ago

Renaming done above, can you verify?

jonathanolson commented 1 year ago

Also, should I uninstrument those?

arouinfar commented 1 year ago

@jonathanolson the updated names and units look good, thanks.

Also, should I uninstrument those?

From our discussion, it sounded like they were necessary to the internal workings of the sim. Rather than uninstrument, we decided to consistently name these properties and provide them with the correct units.

Closing.