Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using NumberControl instead of SliderWithReadout #42

Closed
jessegreenberg opened this issue Jul 1, 2016 · 9 comments
Closed

Consider using NumberControl instead of SliderWithReadout #42

jessegreenberg opened this issue Jul 1, 2016 · 9 comments

Comments

@jessegreenberg
Copy link
Contributor

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
Copy link
Contributor

I'll take care of this one.

@veillette
Copy link
Contributor

The short answer is no, it is not possible (currently) to do so.
I have file an issue in scenery-phet phetsims/scenery-phet#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
Copy link
Contributor

There was already an issue: phetsims/scenery-phet#217

@jessegreenberg
Copy link
Contributor Author

Great, thanks for looking into this @veillette. Comments in #42 (comment) make total sense. Feel free to leave this open or close depending on how you proceed in phetsims/scenery-phet#217.

@ariel-phet ariel-phet assigned pixelzoom and unassigned veillette Aug 23, 2016
@pixelzoom
Copy link
Contributor

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

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 23, 2016

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants