phetsims / density

"Density" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 6 forks source link

Uninstrument `visibleProperty` of blocks radio buttons on Intro Screen #153

Closed arouinfar closed 1 year ago

arouinfar commented 1 year ago

For #150

The Intro screen has a set of RectangularRadioButtons, density.introScreen.view.blocksRadioButtonGroup. image

Hiding individual radio buttons here is problematic. Recently, we have instead uninstrumented the visibleProperty of each button when the group only contains two radio buttons, e.g. viewRadioButtonGroup in CCK: DC and discontinuitiesControl in Calculus Grapher.

Note that there is a blocksRadioButtonGroup on all screens of this sim. This request only applies to the Intro screen which is a set of only 2 radio buttons.

Assigning to @jonathanolson and co-assigning @samreid.

samreid commented 1 year ago

Recently, we have instead uninstrumented the visibleProperty of each button when the group only contains two radio buttons, e.g. viewRadioButtonGroup in CCK: DC and discontinuitiesControl in Calculus Grapher.

Perhaps since this is the 3rd time, and needs to be selectively applied when there are 2 radio buttons only, this should be done in common code. We attempted something similar for enabledProperty in https://github.com/phetsims/sun/issues/826

This feels outside the scope of the current iteration as it pertains to Density (and hence I will self-unassign), but I'll mention it for consideration in https://github.com/phetsims/phet-io/issues/1914

arouinfar commented 1 year ago

Perhaps since this is the 3rd time, and needs to be selectively applied when there are 2 radio buttons only, this should be done in common code

I agree, though from what I recall it was shot down in the same conversation as https://github.com/phetsims/sun/issues/826.

This feels outside the scope of the current iteration as it pertains to Density (and hence I will self-unassign)

Fair enough, thanks @samreid.

@samreid is correct, this should be handled in common code. That said, I think we can pass on this for Density, and maybe someday it will be corrected in sun. I'll go ahead and close.