should values be right aligned?

[pixelzoom]

Noted while I was working on phetsims/scenery-phet#446 (comment) ...

Numeric values with units are displayed throughout this sim using NumberDisplay and NumberControl. And they all appear to use align: 'center' for the value alignment. Using align: 'right' would look better, since the units wouldn't shift around. Examples:

[arouinfar]

Good catch @pixelzoom. Right alignment for these values would definitely look nicer.

Not sure if this would be better to re-assign to @pixelzoom or @jbphet, but feel free to unassign/defer until this sim gets worked on again.

[pixelzoom]

According to responsible_dev.md, @jbphet is responsible for this sim, so I'm going to unassign myself.

[jbphet]

This does seem like an improvement, but it was far more fiddly and time consuming than I'd hoped. I have something implemented, so @arouinfar - please review https://phet-dev.colorado.edu/html/projectile-motion/1.1.0-dev.2/phet/projectile-motion_en_phet.html and let me know if you think any further changes are necessary.

Also, please also opine on whether you feel we should do a maintenance release to get this published. If not, it may be a long time before this goes live.

[pixelzoom]

@jbphet looks like you used the wrong issue number in the commits. I've tagged them above.

[pixelzoom]

@jbphet said:

This does seem like an improvement, but it was far more fiddly and time consuming than I'd hoped. ...

Can you comment on why that was the case? It should be as simple as using NumberDisplay with align: 'right'. Was there a problem with NumberDisplay, or was there something odd done in the sim that required more work?

[jbphet]

Can you comment on why that was the case?

Sure. Several of the cases were not using NumberDisplay, they were creating their own background and putting the number over it. TracerNode.createInformationBox is once such example, and there are several others. In fact, there seemed to be similar methods in several places, which looked like there was some code duplication that could potentially be consolidated, or maybe they should all use NumberDisplay instead. That seemed like a potential time sink, though, and I didn't want to get into it for a sim that has been working well and just needed a visual adjustment. In the TracerNode case, I thought it looked bad to have both the 'no value' readout and the number-with-labels readout right justified, so the code was further complicated (though not much) by handling of that case. The usage of default values was also a little different from what I've seen before. All those little things added up to more time than I originally thought it would take. You'd think I'd know better by now.

Thanks for fixing the commits, not sure how I messed that up...

[pixelzoom]

In the TracerNode case, I thought it looked bad to have both the 'no value' readout and the number-with-labels readout right justified, so the code was further complicated (though not much) by handling of that case.

There's no need to write client code to handle that case. These options to NumberDisplay should have done the job:

align: 'right',
noValueAlign: 'center'

I don't see noValueAlign used in projectile-motion. Did you implement a custom solution?

[pixelzoom]

Several of the cases were not using NumberDisplay, they were creating their own background and putting the number over it. TracerNode.createInformationBox is once such example. ...

I just had a look at this. Highly recommended to replace this (and others) with NumberDisplay.

[jbphet]

I agree that using NumberDisplay is ultimately the way to go. I made a call to adjust rather than rewrite in the interest of time. How about we create another issue and assign to @andrealin to do such an update? I believe she is going to be working with us for a couple of weeks at the beginning of the summer. I'll create that issue now.

[arouinfar]

@jbphet the updated tracer and numberControl readout alignments look good in 1.1.0-dev.2! Thanks for making these changes. I'm not sure that these changes would be critical enough for a maintenance release, so I'll defer to @ariel-phet.

[ariel-phet]

@jbphet @arouinfar @pixelzoom. A nice improvement, but not worthy of a maintenance release. Removing assignees.

[zepumph]

#173 has been created and completed, the values look very nice as right aligned, we decided against an MR. I'm going to close this issue, as this improvement will go out upon next release. Please reopen if you disagree.