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

massNumberProperty and numberControlVolumeProperty need units and consistent names #155

Closed
arouinfar opened this issue Mar 21, 2023 · 5 comments
Assignees

Comments

@arouinfar
Copy link
Contributor

For #150

On the Intro Screen, the Mass NumberControl has an internal massNumberProperty. I decided to unfeature it in #137 because It is identical to the massProperty found in the model. The only usage of massNumberProperty seems to be a dependency for the numberDisplay.valueText.
image

I don't think massNumberProperty or massNumberControl.numberDisplay.valueText add any real value. We use the same mass units in the model and the view. Can these be uninstrumented @jonathanolson?

Similarly, the Volume NumberControl has an associated numberControlVolumeProperty which appears to be the volume in liters, though its units are null. The only usage of numberControlVolumeProperty that I can find is a dependency of the associated numberDisplay.valueText. However, this seems like an odd way to give clients access to the volume in liters. The method in density.compareScreen.view.volumeNumberControl.numberDisplay.valueText.stringProperty is preferable.

@jonathanolson can we instrument numberControlVolumeProperty and use the Compare Screen method to display the NumberDisplay contents?

@jonathanolson
Copy link
Contributor

Added units to the intermediate Properties above.

@arouinfar
Copy link
Contributor Author

Discussed with @jonathanolson and @DianaTavares

The intermediary properties are necessary to the internal workings of the sim, so this is a big ask. Instead, we decided to consistently name these properties, numberControlMassProperty and numberControlVolumeProperty and @jonathanolson has already added units.

jonathanolson added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 27, 2023
@jonathanolson
Copy link
Contributor

Renaming done above, can you verify?

@jonathanolson
Copy link
Contributor

Also, should I uninstrument those?

@arouinfar arouinfar changed the title Uninstrument massNumberProperty and numberControlVolumeProperty massNumberProperty and numberControlVolumeProperty need units and consistent names Mar 27, 2023
@arouinfar
Copy link
Contributor Author

@jonathanolson the updated names and units look good, thanks.

Also, should I uninstrument those?

From our discussion, it sounded like they were necessary to the internal workings of the sim. Rather than uninstrument, we decided to consistently name these properties and provide them with the correct units.

Closing.

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