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

String pattern in SliderWithReadout #44

Closed jessegreenberg closed 8 years ago

jessegreenberg commented 8 years ago

In SliderWithReadout, a string pattern is used to format a value with a unit, but the default pattern is '{0}', so the unit is never added. Is the string pattern still necessary or can it be removed?

  readoutText.text = StringUtils.format( options.patternValueUnit, text, options.unitString );

If it is still necessary, please move the string pattern to plinko-probability-strings_en.json. In options, it is not possible to switch the order of value and unit for right to left languages.

jessegreenberg commented 8 years ago

This issue might be resolved via #42, so that issue should be addressed first.

veillette commented 8 years ago

Regardless of #42, we should fix this. It should be easily done. Assigning to @Denz1994

Denz1994 commented 8 years ago

We were able to specify as an option in the sliderReadoutText that for this sim we only want the numerical value of the readout (according to the design doc). The option for including the unit has been left in the code to generalize how someone might want to include units in the readout text. This will be further investigated by #42.

However, for now we were able to assure a unit would not be included in the readout, thereby making the readout not needed to be translatable because it will always be a number.

Assigning to @jessegreenberg for further review.

jessegreenberg commented 8 years ago

Thanks @Denz1994, that looks good. If this is generalized to common code possibly depending on (https://github.com/phetsims/scenery-phet/issues/244), we will want the default string pattern to come from a translatable strings file (scenery-phet-strings_en.json?)

Could you add a quick note in the documentation reminding us to do this?

jessegreenberg commented 8 years ago

I have added the documentation, will likely be revisited as part of #42. Commits from @Denz1994 look great, closing.