Most/all occurrences of ZoomButton should be converted to use ZoomButtonGroup

[samreid]

In phetsims/circuit-construction-kit-common#620 we generalized ZoomButtonGroup and made it support different icons. Most/all simulations that use ZoomButton duplicate logic regarding layout/listeners/pointer areas and duplicated code around creating both buttons, and most/all should be converted to use this new common code. If no simulations need ZoomButton after this change, it can be deleted. @pixelzoom recommended making a chip-away issue that we track during dev meetings. I'll label for dev meeting so we can touch base and determine priority and game plan.

Simulations using ZoomButton

• blackbody-spectrum @jbphet
• collision-lab @jonathanolson
• energy-skate-park @jessegreenberg
• griddle @samreid
• masses-and-springs @chrisklus
• pendulum-lab @jonathanolson
• ph-scale @pixelzoom
• projectile-motion @zepumph (see Create a container for zoom buttons projectile-motion#245)
• states-of-matter @jbphet
• wave-interference @samreid

[pixelzoom]

First, this issue should probably be titled "... converted to MagnifyingGlassZoomButtonGroup". No one should be converting to ZoomButtonGroup, because it's a base class. And I don't think anyone but me has been using PlusMinusZoomButtonGroup.

Second, I converted ph-scale from ZoomButton to MagnifyingGlassZoomButtonGroup in phetsims/ph-scale#207. I hit several pain points, all of which had to do with the fact that MagnifyingGlassZoomButtonGroup defaults did not look like ZoomButton. I've hopefully addressed those pain points in #653. If you're converting from ZoomButton, MagnifyingGlassZoomButtonGroup should now give you the same "look".

[samreid]

And I don't think anyone but me has been using PlusMinusZoomButtonGroup.

It seems like a design question about whether sims should move to the textual icons or remain using the magnifying glass icons (on a sim-by-sim basis). Sims that have other definitions for "+" and "-" like CCK may require the magnifying glass icons to avoid confusion.

[pixelzoom]

Thanks for clarifying. It wasn't clear to me that considering whether to change from magnifying glasses to +/- icons was included in the scope of this issue. (I don't see a reason to do that for ph-scale.) We should discuss that aspect in dev meeting.

[samreid]

Dev meeting:

We assigned the appropriate developers to this issue for a chip-away. Each developer can determine whether to match the previous style, or to reach out to their designer about switching to other style (the two styles being text +/- vs magnifying glass icons).

@pixelzoom points out that +/- (text form) may be better for graphs.

Also, keep in mind whether other symbols in the sim may confuse--like CCK has "-" charges so we did not use the text "-" zoom buttons.

[jonathanolson]

I've only ever used the zoom amount (e.g. 0.25, 0.5, 1, 2, 4), instead of the zoom level as a Property. Is the zoom level preferred over the amount for phet-io? Otherwise presumably I'd have to include exponential functions whenever the zoom level is used (to convert it to the amount). Any objections if I use a bidirectional DynamicProperty for handling this coordinate frame change?

Additionally, checks don't seem to handle the case where e.g. the zoomLevel+delta would be greater than the max (zoomLevel 1.5, delta 1, max 2, it would improperly try to set the zoomLevel to 2.5). Fractional things seem important for when there is a continuous zoom in place (or is set by phet-io or some other source, even if not in the simulation).

[pixelzoom]

@jonathanolson said:

I've only ever used the zoom amount (e.g. 0.25, 0.5, 1, 2, 4), instead of the zoom level as a Property ...

In that case, you might consider not bothering with this conversion. Imo, different models for "zoom" are totally OK, and there might not be enough value in adapting your model to the semantics of zoomLevelProperty.

In the sims I've developed, each "zoom level" was more complicated than just a multipler value -- it included range, grid line spacing, major tick spacing, minor tick spacing, etc. - hence the need to zoomLevelProperty. When @samreid generalized ZoomButtonGroup, I believe he did so because he had similar semantics for "zoom level".

[samreid]

I've only ever used the zoom amount (e.g. 0.25, 0.5, 1, 2, 4), instead of the zoom level as a Property.

I think the intention from the design was that the zoom levels could be indexed like [0,1,2,3,4,5] and they would correspond to desired zoom levels like [0.25, 0.5, 1, 2, 4]. For instance, I see this pattern in the bamboo example:

https://github.com/phetsims/bamboo/blob/87834b442e228557221b4ef08b7d718c48d0e62a/js/demo/DemoChartCanvasNode.js#L67-L70

Additionally, checks don't seem to handle the case where e.g. the zoomLevel+delta would be greater than the max (zoomLevel 1.5, delta 1, max 2, it would improperly try to set the zoomLevel to 2.5). Fractional things seem important for when there is a continuous zoom in place (or is set by phet-io or some other source, even if not in the simulation).

I think ZoomButtonGroup should be changed to handle the zoomLevel+delta correctly, good idea!

[pixelzoom]

2/25/21 dev meeting:

@zepumph would take on projectile-motion because he's in there for PhET-iO.

@pixelzoom will create sim-specific issues, to be addressed when working on these sims. (Check repo for existing issue before creating.)

[pixelzoom]

Sim-specific issues now exist for all of the sims identified in #652 (comment). They will be addressed when work is done on those sims.

Closing.