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

Should environmentNode.visibleProperty be read-only? #263

Closed
pixelzoom opened this issue Mar 19, 2021 · 8 comments
Closed

Should environmentNode.visibleProperty be read-only? #263

pixelzoom opened this issue Mar 19, 2021 · 8 comments

Comments

@pixelzoom
Copy link
Contributor

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
Copy link
Contributor Author

pixelzoom commented Mar 19, 2021

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

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.

@amanda-phet amanda-phet assigned pixelzoom and unassigned amanda-phet Mar 19, 2021
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 19, 2021

@amanda-phet what do you think about the restructuring of the Studio tree that I proposed in #263 (comment)? Of is just making environmentNode.visibleProperty read-only sufficient?

@amanda-phet
Copy link
Contributor

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.

@amanda-phet amanda-phet removed their assignment Mar 22, 2021
@pixelzoom
Copy link
Contributor Author

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
Copy link
Contributor Author

pixelzoom commented Mar 22, 2021

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 #263 (comment).

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 added a commit that referenced this issue Mar 22, 2021
@pixelzoom
Copy link
Contributor Author

@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
Copy link
Contributor

Looks great! Glad we made the change.

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

2 participants