Rename zoomProperty? - Githubissues

phetsims / projectile-motion

"Projectile Motion" is an educational simulation in HTML5, by PhET Interactive Simulations.

GNU General Public License v3.0

15 stars 13 forks source link

Rename zoomProperty? #276

Open zepumph opened 2 years ago

zepumph commented 2 years ago

While working on https://github.com/phetsims/projectile-motion/issues/275, I had two questions about the zoomProperty, mostly in a PhET-iO context, but also for general maintainability.

Should we rename this to zoomFactorProperty, because I feel like in general it is hard to understand the unit for the Property, when I read the phetioDocumentation, it makes so much more sense: ". . .Each new zoom level increases the value by a factor of 2." Why not just make it more clear in the name?
Now that this Property is in the model, should we update the phetioDocumentation to be more "model-y": "Used to adjust to visual zoom for this screen. Each new zoom level increases the value by a factor of 2."

matthew-blackman commented 1 year ago

Any updates on this? @zepumph @arouinfar

zepumph commented 1 year ago

@arouinfar would have the knowledge about how we do this in other sims. I no longer feel like zoomFactorProperty makes sense. I would comfortably say a sentence like "it has a zoom of 2x" and "factor" would just be confusing. @arouinfar, do you agree? I can update the phetioDocumentation accordingly.

arouinfar commented 1 year ago

When we pick this sim back up, we should follow the pattern used in Geometric Optics. There are two properties related to zoom:

zoomLevelProperty which the integer index controlled by the zoom buttons and is phetioReadOnly: false.
zoomScaleProperty which is derived from zoomLevelProperty and corresponds to the magnification level. It is equivalent to the zoomProperty in Projectile Motion.

Since this sim is currently on hold, I'm going to unassign myself.