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

Are secondaryMassVisibleProperty and secondaryMass.visibleProperty redundant? #25

Closed
samreid opened this issue Aug 10, 2021 · 6 comments

Comments

@samreid
Copy link
Member

samreid commented Aug 10, 2021

During code review #5, I saw this code in DensityExploreModel:

    this.secondaryMassVisibleProperty.link( secondaryMassVisible => {
      this.secondaryMass.visibleProperty.value = secondaryMassVisible;
    } );

Do we need both properties? If so, please explain how they are different and why it is important to have both.

@jonathanolson
Copy link
Contributor

I think this ends up being a phet-io design question. After the recent refactoring to add visibleProperty to individual masses, it would simplify the code to rely on the second masses' visibleProperty. @arouinfar thoughts on whether the top-level model Property is helpful or not?

@arouinfar
Copy link

On the surface, secondaryMassVisibleProperty and secondaryMass.visibleProperty do seem redundant. However, I think they may be somewhat different. The former is hooked up to the RadioButtonGroup responsible for toggling between one and two blocks, for example density.exploreScreen.view.secondMassVisibleControl. In contrast, changing the state of secondaryMass.visibleProperty does not change the state of the RadioButtonGroup. The 2nd block could be hidden while its corresponding control panel is visible (if twoBlocksRadioButton is selected).

In Density and Buoyancy, there are some screens where there are several blocks always present in the play area, such as density.mysteryScreen or buoyancy.introScreen, and other screens where there is a radio button to toggle between either one or two blocks, such as density.exploreScreen and buoyancy.exploreScreen. From a design perspective, it seems cleanest if each block could have its own visibleProperty.

Would it be possible/reasonable to hook up the one vs. two block RadioButtonGroup to secondaryMass.visibleProperty instead of secondaryMassVisibleProperty?

@arouinfar
Copy link

Tagging phetsims/density#76 since it's relevant to the PhET-iO design review.

@jonathanolson
Copy link
Contributor

Handled this for everything except Buoyancy's Shapes screen (which will probably need phet-io design on its own).

@arouinfar how does this look?

@samreid does this satisfy your concern?

@samreid
Copy link
Member Author

samreid commented Aug 11, 2021

Yes, the code change looks good. @arouinfar please close if the design is satisfactory.

@samreid samreid removed their assignment Aug 11, 2021
@arouinfar
Copy link

Looks good to me, thanks @jonathanolson

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