Dynamic layout problem with time control and radio buttons

[pixelzoom]

For code review #83 ...

In all screens, with stringTest=long there is a layout problem with the radio button and the time control:

Very odd that I don't see this problem with ?stringTest=dynamic. So this might be a problem with TimeControlNode.

[AgustinVallejo]

Adding to the meta issue of Dynamic Layout: #122

[pixelzoom]

TimeControlNode looks OK with ?stringTest=long in scenery-phet and plinko-probabiltiy, see screenshots below. So this sim must be doing something odd. See KeplersLawsTimeControlNode.ts.

[pixelzoom]

Here's the culprit in KeplersLawsTimeControlNode.ts:

    this.speedRadioButtonGroupParent!.center = this.getPlayPauseButtonCenter().plusXY(
      2 * PLAY_PAUSE_BUTTON_RADIUS + 2 * STEP_BUTTON_RADIUS + 4 * PUSH_BUTTON_SPACING,
      0
    );

This moves the radio buttons, but doesn't move the push buttons. I don't understand the purpose of this. If the intent is to keep the time control centered, then that should be handled in KeplersLawsScreenView.ts by observing timeControlsNode.boundsProperty. But I see this oddness instead:

   const timeControlsNode = new KeplersLawsTimeControlNode( model,
      {
        enabledProperty: options.playingAllowedProperty || null,
        restartListener: () => model.restart(),
        stepForwardListener: () => model.stepOnce( 1 / 8 ),
        tandem: options.tandem.createTandem( 'timeControlNode' )
      } );
    const timeBox = new AlignBox( timeControlsNode, {
      alignBoundsProperty: this.availableBoundsProperty,
      margin: SolarSystemCommonConstants.MARGIN,
      xAlign: 'center',
      yAlign: 'bottom'
    } );

@AgustinVallejo let's discuss.

[pixelzoom]

I'm guessing that this had to do with the fact that @AgustinVallejo was not aware of boundProperty, or how to use it for dynamic layout. So I went ahead and fixed the layout in the above commit. @AgustinVallejo please review, let me know if you have questions. Close if OK.

[pixelzoom]

Ugh... I see that SolarSystemCommonTimeControlNode.ts has the same problematic code:

    this.speedRadioButtonGroupParent!.center = this.getPlayPauseButtonCenter().plusXY(
      -0.9 * ( PLAY_PAUSE_BUTTON_RADIUS + STEP_BUTTON_RADIUS ),
      PLAY_PAUSE_BUTTON_RADIUS + STEP_BUTTON_RADIUS + 3 * PUSH_BUTTON_SPACING
    );

And SolarSystemCommonTimeControlNode and KeplersLawsTimeControlNode are identical. So one of them needs to go away, and this problem probably needs to be fixed eagerly in My Solar System.

[pixelzoom]

It gets even weirder.

In SolarSystemCommonScreenView:

const timeControlNode = new SolarSystemCommonTimeControlNode( model,

KeplersLawsScreenView extends SolarSystemCommonScreenView, and adds another time control:

class KeplersLawsScreenView extends SolarSystemCommonScreenView {
...
    const timeControlsNode = new KeplersLawsTimeControlNode( model, {

What is going on here?...

[pixelzoom]

In https://github.com/phetsims/keplers-laws/issues/141, @AgustinVallejo consolidate the duplicate time controls. I verified that it's still behaving correctly with dynmic strings. So this issue can be closed.