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

Uninstrument visibleProperty of blocks radio buttons on Intro Screen #153

Closed
arouinfar opened this issue Mar 21, 2023 · 2 comments
Closed
Assignees

Comments

@arouinfar
Copy link
Contributor

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
Copy link
Member

samreid commented Mar 24, 2023

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 phetsims/sun#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

@samreid samreid removed their assignment Mar 24, 2023
@arouinfar
Copy link
Contributor Author

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 phetsims/sun#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.

@arouinfar arouinfar closed this as not planned Won't fix, can't repro, duplicate, stale Mar 27, 2023
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

3 participants