search

phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

Make the pipes not interactive (enabledProperty) #243

Closed marlitas closed 4 months ago

marlitas commented 4 months ago

Just make meanShareAndBalance.levelOutScreen.view.controls.pipeSwitch.toggleSwitch.enabledProperty control the interactive valves at the same time

marlitas commented 4 months ago

Done above. Over to @amanda-phet and @jbphet for review.

amanda-phet commented 4 months ago

I'm a bit confused.

meanShareAndBalance.levelOutScreen.model.arePipesEnabledProperty is working to control whether pipes are enabled by clicking the valve directly or the toggle.

meanShareAndBalance.levelOutScreen.view.controls.pipeSwitch.toggleSwitch.enabledProperty only controls the toggle switch.

Is this OK? It doesn't match what you wrote above, but we still achieved the goal of being able to disable all pipes in one location.

marlitas commented 4 months ago

@amanda-phet that unfortunately is part of the over-instrumentation of some common code components @jbphet mentioned earlier. If you look at meanShareAndBalance.levelOutScreen.view.controls.pipeSwitch.enabledProperty, you'll see that is linked to the same 'enabledProperty' each individual pipeNode has as well.

I unfortunately can't do anything about meanShareAndBalance.levelOutScreen.view.controls.pipeSwitch.toggleSwitch.enabledProperty

amanda-phet commented 4 months ago

Got it. Just wanted to be sure all was working as we want it to.

jbphet commented 4 months ago

This mostly looks good. The only question I have is why the model was changed to use names like pipesOpenProperty instead of arePipesOpenProperty, but several of the objects to which these Properties are passed are using the older arePipesOpenProperty name. If this wasn't by design, it would be good to use the newer, shorter name in all places, since it would make the code easier to understand. If it is intentional, it would be good to have some documentation to explain why.

marlitas commented 4 months ago

Oh good catch! That's left over from changes in https://github.com/phetsims/mean-share-and-balance/issues/256. I'll go ahead and address.

marlitas commented 4 months ago

I updated the names mentioned above. I believe this issue is ready to close.