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

Link associated Nodes and checkboxes to the same visibleProperty #148

Closed
pixelzoom opened this issue Mar 4, 2021 · 3 comments
Closed

Link associated Nodes and checkboxes to the same visibleProperty #148

pixelzoom opened this issue Mar 4, 2021 · 3 comments

Comments

@pixelzoom
Copy link
Contributor

3/4/21 design meeting change request, related to #137 and #139

For all Nodes whose visiblity is controlled via a checkbox, link them to the same Property that is controlled by the checkbox. For example, rootsNode, vertextNode, ...

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 5, 2021

The only element that was capable of using a checkbox Property directly was graphingQuadratics.focusAndDirectrixScreen.view.pointOnParabolaManipulator.visibleProperty.

Visibility of other Nodes was derived from a checkbox Property and some other state. For example, vertextNode is only visible if the checkbox is checked and the curve has a vertex.

What I did discover is many instances of setting some visibleProperty to phetioReadOnly: true specified in overrides.js. I moved all of them to code. Here's why:

Anything that is derived in the code should be specified as phetioReadOnly: true in the code. That's the case for for these elements:

graphingQuadratics.focusAndDirectrixScreen.view.focusManipulator.visibleProperty
graphingQuadratics.focusAndDirectrixScreen.view.vertexManipulator.visibleProperty
graphingQuadratics.standardFormScreen.view.noRealRootsNode.visibleProperty
graphingQuadratics.standardFormScreen.view.rootsNode.visibleProperty
graphingQuadratics.standardFormScreen.view.vertexNode.visibleProperty
graphingQuadratics.vertexFormScreen.view.vertexManipulator.visibleProperty

phetioReadOnly: true was duplicated 4 times each for these buttons, which appear in all screens. It makes sense to move this to the code, so that one screen isn't accidently set differently.

equationAccordionBox.buttonGroup.eraseButton.visibleProperty
equationAccordionBox.buttonGroup.saveButton.visibleProperty

So... @amanda-phet please review. Here's what you need to verify:

  • *.pointOnParabolaManipulator.visibleProperty is linked to *.viewProperties.pointOnParabolaVisibleProperty
  • All of the elements enumerated above are read-only

@amanda-phet
Copy link
Contributor

Verified the above.

I am a little confused- First, did @kathy-phet and I set these to as phetioReadOnly: true in overrides.js? I don't remember that, nor did I realize we could do that. I thought we could only specify the featured items in that file.

Second, are these ReadOnly because they can be set by the client within the sim by just checking the box? I can't remember why we'd want these to be ReadOnly.

@amanda-phet amanda-phet assigned pixelzoom and unassigned amanda-phet Apr 28, 2021
@amanda-phet
Copy link
Contributor

It appears my questions are answered in #149 .

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