search

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

Create a container for zoom buttons #245

Closed arouinfar closed 2 years ago

arouinfar commented 3 years ago

For #244

Here's the current tree structure for the zoom buttons.

It would be nice if the zoom buttons could be grouped into something like zoomButtonGroup. That way, there could be a top-level visibleProperty to hide both buttons at once. It's not particularly useful to hide one at a time, and a bit annoying to hide two buttons instead of a group. See phScale.microScreen.view.graphNode.linearGraphNode.zoomButtonGroup for an analogous example.

That still leaves the question of where zoomProperty belongs. It's a sibling to the individual zoom buttons, so it could probably go in the zoomButtonGroup container, or perhaps it would be more appropriately housed in the viewProperties proposed in #224. I don't have any particular preference, so whatever @zepumph thinks is best.

arouinfar commented 3 years ago

@zepumph according to https://github.com/phetsims/scenery-phet/issues/652, this sim should be using ZoomButtonGroup. Perhaps that will take care of the tree structure.

zepumph commented 3 years ago

Right @arouinfar. I will take care of this by converting to ZoomButtonGroup.

zepumph commented 3 years ago

This wasn't a direct conversion because until my recent work in https://github.com/phetsims/scenery-phet/issues/687, ZoomButtonGroup didn't support the *= operation that this sim's zoom ran on. Now that it does, this should be easy and straight forward.

zepumph commented 3 years ago

@arouinfar, this refactor has been done by using MagnifyingGlassZoomButtonGroup. The one question I had while doing this is wondering if the ZoomButtonGroup should have a linked element pointing to where the zoomButton is instrumented. In the case of Projectile Motion, the Property is right next to the group, so it doesn't really matter, but that isn't necessarily always the case. Over to you for a general review and to answer my question.

arouinfar commented 3 years ago

The ZoomButtonGroup looks good in master. It's a bit odd to have a lone property hanging out under view. Can you move it to viewProperties.zoomProperty and add a linked property to ZoomButtonGroup?

zepumph commented 3 years ago

Everything has been done and went smoothly.

and add a linked property to ZoomButtonGroup?

Added, this is common code that updated the API for Natural Selection as well (in these commits).

arouinfar commented 2 years ago

Thanks @zepumph. The ZoomButtonGroup looks great in master.