search

phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

Should environmentNode.visibleProperty be read-only? #263

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

I noticed that *.environmentNode.visibleProperty is not currently read-only. When it's set to false, it looks odd, see screenshot below. Should it be read-only?

screenshot_210

pixelzoom commented 3 years ago

If for some reason we need to support hiding the environmentNode and everything that's on top of it (clock, radio buttons, push buttons) then we will need to add an additional parent element for all of those things, environmentPanel.

Studio tree is currently:

screenshot_211

It would change to:

view
  visibleProperty
  +addMutationsPanel
  +dialogs
  +environmentalFactorsPanel
  +environmentPanel
    +environmentNode
    +environmentRadioButtonGroup
    +generationClock
    +playButtonGroup
  +genes
  +graphs
  +resetAllButton
  +timeControlNode

environmentPanel.visibleProperty would be writeable. environmentNode.visibleProperty would be changed to read-only

amanda-phet commented 3 years ago

Yikes! I don't know how we didn't notice this before when we reviewed studio. Let's definitely make this read-only. I can't think of a reason to want to set environmentNode.visibleProperty to false.

pixelzoom commented 3 years ago

@amanda-phet what do you think about the restructuring of the Studio tree that I proposed in https://github.com/phetsims/natural-selection/issues/263#issuecomment-802978938? Of is just making environmentNode.visibleProperty read-only sufficient?

amanda-phet commented 3 years ago

I prefer to not restructure the tree that way. I also think it would be incredibly strange for someone to want to hide the entire environment (or environmentPanel) in this sim. Without it, the sim is not very useful.

pixelzoom commented 3 years ago

OK, no environmentPanel, and n changes to the tree structure.

I have made environmentNode.visibleProperty read-only. @amanda-phet please review in master, then please close.

pixelzoom commented 3 years ago

Re the tree structure... @amanda-phet and I discussed this more on Slack. We discussed how this part of the UI might be interpretted by the instructional designer:

screenshot_923

Is it a panel with the clock, radio buttons, and push buttons on top of it? (siblings in the tree, the current implementation) Or is it a panel, with the clock, radio buttons, and push buttons inside of it? (children of the panel in the tree) We decided that the latter made more sense, and that's the structural change that I describe in https://github.com/phetsims/natural-selection/issues/263#issuecomment-802978938.

This is a relatively easy change, and is nice structural improvement, so I will make it so. And environmentPanel.visibleProperty will be read-only.

pixelzoom commented 3 years ago

@amanda-phet ready for review. The revised tree structure is shown below. And these 2 elements are now read-only:

environmentPanel.visibleProperty environmentPanel.environmentNode.visibleProperty

screenshot_924
amanda-phet commented 3 years ago

Looks great! Glad we made the change.