ControlPanel layout

[jessegreenberg]

While fixing #105, we noticed some slightly strange layout within the control panel for translated strings.

@zepumph and I tried a few different layout strategies including using different VBoxes, and AlignGroup with AlignBoxes. But it didn't work out because the different slider components have no knowledge of each other. I order to perfectly align each row, we would like to change SliderUnit (or completely do away with it) so that the control panel can manage layout more directly.

@zepumph please fill in or correct any of this info if I am missing something.

@ariel-phet do you see this layout as problematic and do you think it is worth time fixing?

[jessegreenberg]

Here is the layout with default strings

[ariel-phet]

@jessegreenberg yes we should fix these layout issues. We have been picky about these types of layout issues as a rule for i18n, so I do not think we should make an exception here.

[jessegreenberg]

Ok, thanks. We should do this before the upcoming redeploys.

[jessegreenberg]

@zepumph here is what I am thinking: Keep SliderUnit, but instead of making it responsible for layout, it just collects the components and provides an API to get them in the control panel. Then, in the control panel we use getters to get valueText, unitText, and so on. The control panel could create some align boxes that guarantees that rows are aligned.

In control panel:

var unitAlignBox = new AlignBox( { matchHorizontal: false } )
var resistanceUnitsBox = unitAlignBox.createBox( resistanceSliderUnit.unitsText );
var lengthUnitsBox = unitAlignBox.createBox( lengthSliderUnit.unitsText );
var areaUnitsBox = unitAlignBox.createBox( lengthSliderUnit.unitsText );

// horizontally aligned units, bottom aligned to manage sup text
var unitsHBox = new HBox( { children: [ resistanceUnitsBox, lengthUnitsBox, areaUnitsBox ], align: 'bottom' );

[zepumph]

that's a good idea. That way we will be able to align them all into a grid very nicely.

[jessegreenberg]

Alright, thanks Ill give it a shot.

[jessegreenberg]

Before using AlignGroup, I tried again to do the layout relative to the bottom text for consistency and it worked out.

@zepumph would you please review? The remaining problem is with the unit text for stringTest=double, which I believe is a bounds problem with scenery RichText (https://github.com/phetsims/scenery/issues/687). @ariel-phet does this require a fix before the redeploy?

In the deployed version, the ohmcm move down a little with stringTest=double as well: https://phet.colorado.edu/sims/html/resistance-in-a-wire/latest/resistance-in-a-wire_en.html?stringTest=double

[zepumph]

Nice! I didn't realize that this was a know bug, and already in the published version. @jessegreenberg thanks for diving in again. To me it looks good.

[ariel-phet]

@jessegreenberg sorry I am a little unclear on what aspect this question is referring to?

@ariel-phet does this require a fix before the redeploy?

[jessegreenberg]

Sorry, the slight misalignment of the bottom row of units text with stringTest=double, shown in the second image in https://github.com/phetsims/resistance-in-a-wire/issues/106#issuecomment-335970449.

[ariel-phet]

@jessegreenberg - a quick scan of the translations of the current published version show that the majority of the translations do not change the units.

However, a few do, such as https://phet.colorado.edu/sims/html/resistance-in-a-wire/latest/resistance-in-a-wire_kn.html

You will see this slight misalignment is present in the published version (it seems acceptable)

So, it would be ideal to fix before redeploy, but my understanding is that you are under a fair bit of a time crunch to get more of these sims out. So skipping this fix would be acceptable, as it is not causing a major bug/horrible aesthetics.

[jessegreenberg]

Sounds great, thanks @ariel-phet. I think the fix will require work in https://github.com/phetsims/scenery/issues/687, so we will see what @jonathanolson suggests before deciding.

[jessegreenberg]

@jonathanolson provided a recommendation that is working nicely in this case, thanks!

[jessegreenberg]

Fixed in the above commit, this can be closed.

[zepumph]

@jessegreenberg @jonathanolson you two are heros. Thanks for cracking down on this. I think next Ohms Law will need to be handled in the same manner. I'll make an issue.