Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pixelzoom opened this issue Nov 27, 2023 · 2 comments
Open

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

pixelzoom opened this issue Nov 27, 2023 · 2 comments

Comments

@pixelzoom
Copy link
Contributor

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 phetsims/my-solar-system#301.

(2) It has dynamic layout problems. See problem description and recommended solution in phetsims/my-solar-system#300.

It's unclear whether this is blocking for MSS PhET-iO 1.3 (phetsims/my-solar-system#282).

@pixelzoom
Copy link
Contributor Author

Regarding the dynamic layout problem...

It looks like others may have encountered this previously. In 1927a55 for #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
Copy link
Contributor Author

pixelzoom commented Nov 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants