phetsims / fluid-pressure-and-flow

"Fluid Pressure and Flow" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
8 stars 5 forks source link

design issues with ControlSlider #65

Open pixelzoom opened 10 years ago

pixelzoom commented 10 years ago

A few issues with ControlSlider...

  1. It's poorly named. As its documentation states, it is a slider + buttons "for changing fluid density and gravitation". So it's a combination of multiple controls (not a slider) and it's not general, but specific to one attribute. Recommended to rename it to FluidDensityControl.
  2. It has enable and disable function which do not appear to be used. If they're not used, recommend to delete them. For future implementations: an 'enabled' property would be a the preferred interface; an ES5 'enabled' field would be a second option; a setEnabled({boolean} prototype function would be a distance third options; separate enable/disable function are not recommended.
  3. Constructor parameters 'trackProperty' and 'trackRange' are poorly named, especially when there's a slider involved. They sound like something related to the slider's track. Recommended to change them to fluidDensityProperty and fluidDensityRange.
  4. Constructor parameter getPropertyStringFunction is a bit confusing, and 'Function' in the name is redundant. Recommended valueToString.
  5. Replace the reset function with an additional constructor parameter, {Property} expandedProperty. This should be a view property, created in WaterTowerScreenView. For an example of a view-specific PropertySet, see ph-scale.MacroView.
  6. Related constructor parameters should be next to each other, specifically fluidDensityProperty and fluidDensityRange.

So... After all of the above changes, your constructor should look like this:

/**
 * @param {WaterTowerModel} waterTowerModel
 * @param {Property<number>} fluidDensityProperty fluid density, in kg/m^3
 * @param {Range} fluidDensityRange range of fluid density value
 * @param {Property<boolean>} expandedProperty determines whether the control is expanded
 * @param {Function} valueToString returns the fluid-density value as a string
 * @param {*} options
 * @constructor
 */
function FluidDensityControl( waterTowerModel, fluidDensityProperty, fluidDensityRange, expandedProperty, valueToString, options )
samreid commented 10 years ago

This code was copied from Under Pressure, and I should be responsible for fixing it. Assigned to @samreid.

samreid commented 10 years ago

Perhaps @notsiddhartha could work on this as part of the job after "Flow" is done.

samreid commented 10 years ago

Assigned to @ariel-phet to see if we want to request a quote (as part of "under pressure") or implement this ourselves.