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

Consider using NumberControl instead of SliderWithReadout #42

Closed jessegreenberg closed 8 years ago

jessegreenberg commented 8 years ago

SliderWithReadout is quite similar to a new scenery-phet element called NumberControl. Could the common code element replace SliderWithReadout? If not, perhaps we could determine what options need to be added to NumberControl so that it could be used in plinko-probability.

veillette commented 8 years ago

I'll take care of this one.

veillette commented 8 years ago

The short answer is no, it is not possible (currently) to do so. I have file an issue in scenery-phet https://github.com/phetsims/scenery-phet/issues/244 and I'll bring it up to the dev meeting. It turns out that many sims have a similar layout to plinko-probability.

For your information, I wrote SliderWithReadout nearly two years ago. At that point we didn't have NumberControl. I attempted to write it in such a general way that it could become a common component. (which explains why there are so many options, many of which are not used in plinko-probability)

veillette commented 8 years ago

There was already an issue: https://github.com/phetsims/scenery-phet/issues/217

jessegreenberg commented 8 years ago

Great, thanks for looking into this @veillette. Comments in https://github.com/phetsims/plinko-probability/issues/42#issuecomment-230871937 make total sense. Feel free to leave this open or close depending on how you proceed in https://github.com/phetsims/scenery-phet/issues/217.

pixelzoom commented 8 years ago

phetsims/scenery-phet#217 is closed, so this can now be addressed.

pixelzoom commented 8 years ago

@ariel-phet assigned this to me, to replace SliderWithReadout with NumberControl.

Done, also fixed an i18n layout issue. I made the value font a tad larger, since it seemed awfully small compared to other displayed values. Screenshot below. @jessegreenberg would you mind reviewing?

screenshot_86

jessegreenberg commented 8 years ago

The code looks great @pixelzoom. I notice that the NumberControl thumbs are now green rather than the default HSlider light blue.

The green is coming from the default for thumbFillEnabled in NumberControl. Can NumberControl use the default HSlider thumb color?

pixelzoom commented 8 years ago

Yes, I think that NumberControl should use the default HSlider thumb color. I've done that in the above commit. The only clients are currently hookes-law and CCK. I've verified that it doesn't change hookes-law.

@samreid Please be aware that NumberControl is now using the default color for the HSlider thumb, rather than setting its own default of 'green'. If you were relying on 'green', you'll need to set thumbFillEnabled:'green' explicitly.

pixelzoom commented 8 years ago

I compared with an older dev version, and the thumb color is now correct. Closing.