Can MassControl be replaced by a number control?

[samreid]

Discovered during #78, I noticed that MassControl has 129 lines of code to render this:

Can we instead use some pre-existing number control?

[samreid]

Here are some pre-existing number controls from scenery-phet:

[samreid]

Using NumberControl, I have nearly matched the look of the original:

To Do:

1. black stroke around text box
2. fix text box width
3. center text in text box
4. fix i18n

[samreid]

I noticed this in the old code. How important is this to be a floor and not a round?

valueLabel.text = StringUtils.format( pattern0Value1UnitsString, Math.floor( value ), unitsKgString );

[samreid]

First commit above, lint is passing, most features working.

(3) center text in text box

I no longer recommend to do this, after testing with right-aligned it seems to better convey the change in numbers.

To Do:

1. black stroke around text box
2. fix text box width

[samreid]

@pixelzoom said:

[11/14/16, 2:49:33 PM] Chris Malley: I'm adding 2 options, to be passed to NumberDisplay:
[11/14/16, 2:49:41 PM] Chris Malley: valueBackgroundStroke: 'lightGray',
valueBackgroundLineWidth: 1,

[samreid]

Here's the original:

Here's how far I got by eliminating duplicated code and Reusing NumberControl:

@ariel-phet is this close enough? You can test on phettest.colorado.edu to see it in action.

Should we slightly round the corners of the text box? Anything else to change?

EDIT: when making recommendations, please be clear whether (a) they should be changed directly in NumberControl and thus fixed everywhere NumberControl appears or (b) leave the defaults alone in NumberControl, but generalize it so it can be different for this case.

[ariel-phet]

@samreid

1. Yes we should round the corners of the text box (changed directly in NumberControl)
2. The original has a bit more negative space between the text box and the tweakers, I think that is aesthetically preferable (leave defaults in NumberControl, but generalize so this case can be accomodated)
3. In the original text box the value and unit remain centered in the text box as the value grows and shrinks. I am not sure if this is necessary (it seems nice aesthetically, but I don't think it causes any usability issues or such). However, noting the difference. it seems like we might want to at least have the dynamic centering as an option in NumberControl.

[samreid]

Sounds good, thanks for the review.

In the original text box the value and unit remain centered in the text box as the value grows and shrinks. I am not sure if this is necessary (it seems nice aesthetically, but I don't think it causes any usability issues or such). However, noting the difference. it seems like we might want to at least have the dynamic centering as an option in NumberControl.

After experimenting with both, I realized it much easier to visually understand the orders of magnitude in the "right-aligned" one, because the "kg" marker stays in the same spot and you can see the numbers bumping to the left as they go up by orders of magnitude. In the centered strategy (which I originally thought looked more appealing), this effect is washed out. But I agree we should add this as an option for potential usage in other contexts.

[pixelzoom]

1. The original has a bit more negative space between the text box and the tweakers, I think that is aesthetically preferable (leave defaults in NumberControl, but generalize so this case can be accommodated)

Use NumberControl option layoutFunction: NumberControl.createLayoutFunction1( { ySpacing: YOU_VALUE } )

1. ... it seems like we might want to at least have the dynamic centering as an option in NumberControl.

Use NumberControl option valueAlign:'center', added in above commit.

[samreid]

Yes we should round the corners of the text box (changed directly in NumberControl)

@pixelzoom argued that the amount of rounding should remain application specific, since it is in pixels, and he recommended to just supply the corner radius from the sim. Hence I'll proceed with that for now.

[samreid]

I changed the spacing from the default of 5 to 10.

Old one:

New one:

[samreid]

@ariel-phet would you like to review?

[ariel-phet]

@samreid looks good to me. However, we should also check with @arouinfar since she is the lead designer for this sim. @arouinfar this looks pretty good to me but I would have two main questions unless you see other tweaks:

1. Are you OK with the readout being fixed, (not dynamically centering as in the published version). Take a look on phettest to see the difference
2. What should the alignment relationship be between the sliders and tweakers. In @samreid new one he has aligned the tweakers to the outer edges of the tick mark labels (it seems the other obvious choice is aligning to to the edge tick marks)
3. it seems to me the tick mark labels should be a bit closer to the tick marks (that would be a slight tweak over the original).

[arouinfar]

In general, things look pretty good to me @samreid @ariel-phet.

Are you OK with the readout being fixed, (not dynamically centering as in the published version). Take a look on phettest to see the difference

It looks fine to me, since the text at max width is centered within the readout.

What should the alignment relationship be between the sliders and tweakers. In @samreid new one he has aligned the tweakers to the outer edges of the tick mark labels (it seems the other obvious choice is aligning to to the edge tick marks)

At first glance it looks reasonable, though when comparing the slider track the alignment seems a bit off. I have a slight preference for aligning the tweakers to the tick marks instead.

it seems to me the tick mark labels should be a bit closer to the tick marks (that would be a slight tweak over the original).

Agreed. I think 1 or 2 px would do the trick.

[samreid]

In the above commit, I reduced the tickLabelSpacing from 6 to 2, it now looks like this:

@arouinfar anything else to do here? You can test in context on phettest if you wish.

[arouinfar]

Looks good to me @samreid.