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

default options need adjusting in ZoomButtonGroup hierarchy #653

Closed
pixelzoom opened this issue Dec 17, 2020 · 5 comments
Closed

default options need adjusting in ZoomButtonGroup hierarchy #653

pixelzoom opened this issue Dec 17, 2020 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 17, 2020

Related to #652. Something I noticed as I'm converting ph-scale to MagnifyingGlassZoomButtonGroup in phetsims/ph-scale#206...

ZoomButton uses buttonAppearanceStrategy: RectangularButton.ThreeDAppearanceStrategy (the default), while ZoomButtonGroup uses buttonAppearanceStrategy: RectangularButton.FlatAppearanceStrategy.

If we want to convert sims from ZoomButton to MagnifyingGlassZoomButtonGroup, and have them look the same as they do now, that means that every sim is going to have to set buttonAppearanceStrategy: RectangularButton.ThreeDAppearanceStrategy for MagnifyingGlassZoomButtonGroup. And we don't want to do that.

So I think the buttonAppearanceStrategyoverride should be moved from ZoomButtonGroup to PlusMinusZoomButtonGroup, so that MagnifyingGlassZoomButtonGroup uses 3D buttons. And if @samreid needs them to be flat in CCK:AC, then overridebuttonAppearanceStrategy` there.

@pixelzoom pixelzoom self-assigned this Dec 17, 2020
@pixelzoom
Copy link
Contributor Author

Ditto for cornerRadius. cornerRadius: 0 is specific to PlusMinusZoomButtonGroup, it doesn't belong in ZoomButtonGroup. Every sim ported to MagnifyingGlassZoomButtonGroup will be forced to set cornerRadius: 4 to keep the same look.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 17, 2020

Ditto for baseColor. MagnifyingGlassZoomButtonGroup should default to buttonOptions.baseColor: PhetColorScheme.BUTTON_YELLOW in order have the same look as ZoomButton. Otherwise every converted sim will need to set baseColor.

@pixelzoom
Copy link
Contributor Author

Ditto for magnifyingGlassOptions.glassRadius. MagnifyingGlassZoomButtonGroup uses 10, ZoomButton uses 15. I'm guessing the values here were set for CCK:AC. They need to be set to match ZoomButton, or converting sims from ZoomButton to MagnifyingGlassZoomButtonGroup is going to be a pain.

@pixelzoom pixelzoom changed the title buttonAppearanceStrategy needs adjusting in ZoomButtonGroup hierarchy options needs adjusting in ZoomButtonGroup hierarchy Dec 17, 2020
@pixelzoom pixelzoom changed the title options needs adjusting in ZoomButtonGroup hierarchy default options need adjusting in ZoomButtonGroup hierarchy Dec 17, 2020
pixelzoom added a commit to phetsims/circuit-construction-kit-common that referenced this issue Dec 17, 2020
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 17, 2020

In the above commits, I've adjusted the default options so that:

  • PlusMinusZoomButtonGroup has buttons that are flat, white, square corners.
  • MagnifyingGlassButtonGroup has buttons that default to the look of ZoomButton (3D, yellow, rounded corners,...) This makes it easier for sims to migrate from ZoomButton to MagnifyingGlassButtonGroup.
  • CCK:AC overrides defaults for MagnifyingGlassButtonGroup to provide the "look" that's desired in that sim.

@samreid please review.

@samreid
Copy link
Member

samreid commented Dec 28, 2020

I reviewed the commits and tested the default components in the scenery-phet harness, and tested the buttons in CCK-AC. Everything looks good to me, thanks! Closing.

@samreid samreid closed this as completed Dec 28, 2020
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