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

Change requests for graphingQuadratics.standardFormScreen.view.rootsNode #146

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

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 2, 2021

For #137 and #139...

graphingQuadratics.standardFormScreen.view.rootsNode (RootsNode.js):

  • When there is 1 root, rightCoordinatesProperty should have the same value as leftCoordinatesProperty (it's currently null).
  • rightRootNode should still be invisible if there is 1 root.
  • rightCoordinatesProperty should have same initial values as leftCoordinatesProperty.
  • Verify what happens with leftRootNode and rightRootNode if you set visibleProperty=true when there are no roots.
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 3, 2021

Summary of changes in the above commits:

  • When there is 1 root, leftCoordinatesProperty and rightCoordinatesProperty are the same.
  • leftCoordinatesProperty and rightCoordinatesProperty are both intiialzed to (0,0)
  • Updated phetioDocumentation for leftCoordinatesProperty and rightCoordinatesProperty
  • Updates phetioDocumentation for leftRootNode and rightRootNode

Testing in Studio:

  • Inspected leftCoordinatesProperty and rightCoordinatesProperty
  • Inspected phetioDocumentation
  • Verified that with no roots, setting visibleProperty=true for leftRootNode and rightRootNode does not result in anything being displayed. Note that visibleProperty will automatically change when the roots change, so changing visibleProperty does not result in a persistent change. So we might consider making visibleProperty phetioReadOnly: true.

@pixelzoom
Copy link
Contributor Author

@amanda-phet ready for review in master or 1.2.0-dev.1.

Note this possible change in #146 (comment):

... Note that visibleProperty will automatically change when the roots change, so changing visibleProperty does not result in a persistent change. So we might consider making visibleProperty phetioReadOnly: true.

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

Note that visibleProperty will automatically change when the roots change, so changing visibleProperty does not result in a persistent change. So we might consider making visibleProperty phetioReadOnly: true.

graphingQuadratics.standardFormScreen.view.rootsNode.leftRootNode.visibleProperty already appears to be read only, so I'm not sure I follow. We do what this to be read only, so it seems to be working correctly.

I confirmed that the roots are behaving as requested, which is great!

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