search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

TimeControlNode needs work for PhET-iO and dynamic layout #826

Open pixelzoom opened 7 months ago

pixelzoom commented 7 months ago

Identified by @arouinfar during review of MSS PhET-iO. And according to @matthew-blackman, this will also be an issue for PM"DL.

TimeControlNode has 2 significant problems for PhET-iO:

(1) It needs a PhET-iO design. It's over-instrumented, and doesn't support opting out/in of instrumented some subcomponents. See for example https://github.com/phetsims/my-solar-system/issues/301.

(2) It has dynamic layout problems. See problem description and recommended solution in https://github.com/phetsims/my-solar-system/issues/300.

It's unclear whether this is blocking for MSS PhET-iO 1.3 (https://github.com/phetsims/my-solar-system/issues/282).

pixelzoom commented 7 months ago

Regarding the dynamic layout problem...

It looks like others may have encountered this previously. In https://github.com/phetsims/scenery-phet/commit/1927a5556fa11db6ee7127584dd687ac811a5985 for https://github.com/phetsims/scenery-phet/issues/575, @jessegreenberg added options wrapSpeedRadioButtonGroupInPanel and speedRadioButtonGroupPanelOptions, presumably because wrapping the radio button group in a panel makes the dynamic layout behave properly. Those options were used only in neuron, but now we also needed to use them in solar-system-common (my-solar-system, keplers-laws).

PhET-iO makes this a problem for all sims. So I recommend that those options be deleted, and the dynamic layout be generalized so that is always works properly.

pixelzoom commented 7 months ago

Regarding the positioning of the speed radio button group, I recommend replacing option speedRadioButtonGroupOnLeft: boolean with speedRadioButtonGroupPosition: 'left' | 'right' | 'bottom' ('top' is probably not necessary), then handling the layout accordingly.

The ugly/unsafe workaround for 'bottom' placement in SolarSystemCommonTimeControlNode should then be deleted.