Zoom buttons need magnifying glasses

[samreid]

From today's design meeting, "+" and "-" are overloaded in CCK, and showing those as button icons without other context will be confusing. We need the magnifying glass icons. This is for the charts and for the play area zoom buttons.

@ariel-phet also indicated the flat buttons are good.

[samreid]

The play area zoom buttons already have magnifying glasses, so I'll move this to the AC milestone.

[samreid]

I opened phetsims/scenery-phet#647, but the play area zoom buttons look like this:

Should the chart zoom buttons look identical to the play area zoom buttons? If so, what styling/coloring should we use?

[samreid]

I discussed this with @pixelzoom in phetsims/scenery-phet#647 and on slack. His opinion was that the +/- buttons are already perfectly clear in context, and that adding the complexity of changing icons for the ZoomButtonGroup is unwarranted.

I recommend we do one of the following (no particular order):

1. Spend the time to generalize ZoomButtonGroup so that it can support arbitrary icons
2. Accept the "+"/"-" text icons

I'd recommend against a custom or one-off solution for this case.

@arouinfar @ariel-phet @kathy-phet can you please recommend how to proceed?

[pixelzoom]

If you add the ability to use magnify glass icons, you'll also need to factor that icon out of ZoomButton.

Or even better, switch to using the fontawesome icons for the magnifying glasses:

https://fontawesome.com/icons/search-plus?style=solid
https://fontawesome.com/icons/search-minus?style=solid

[samreid]

Would it be OK if ZoomButtonGroup was hardcoded to use the magnifying glass icons?

[pixelzoom]

Do you mean as an option, or instead of +/- ?

[pixelzoom]

And I'm still trying to understand... If you want the ZoomButton look (magnifying glasses) why not just use a pair of ZoomButtons? That's what has been done in other sims that use "magnifying glass" zoom buttons. What's special about CCK, and why does it need to use ZoomButtonGroup? Why create a 3rd variation on zoom buttons?

[samreid]

Do you mean as an option, or instead of +/- ?

It would simplify things if we can use the same icons everywhere. But if we need flexibility, then we should make it possible/easy to specify any icon.

why not just use a pair of ZoomButtons

ZoomButtonGroup adds:

• Layout
• Pointer areas
• Disposal
• Enabled
• Deltas

Why should this be duplicated?

[pixelzoom]

It would simplify things if we can use the same icons everywhere.

Changing the icons to magnifying glasses is not OK with me, strong pushback. The primary reason for writing ZoomButtonGroup was to provide a cleaner, more modern look to zoom buttons, similar to what's used in many web interfaces (Google maps, etc.) -- intentionally and specifically without magnifying glasses.

[pixelzoom]

If you want to add the ability to show magnifying glass icons to ZoomButtonGroup, you'll need at least 2 options:

• An option to indicate whether to show magnifying glasses or +/-, probably something clunky like useMagnifyingGlassIcons.
• An option to scale the FontAwesomeNodes that display the magnifying glass icons. Either something like magnifyingGlassIconsScale or fontAwesomeOptions if you want to go the nested-options approach.

Then you'll have the problem that some options (e.g. textOptions and fontAwesomeOptions) are ignored depending on which the type of icons displayed. Again, clunky.

Another approach would be to make ZoomButtonGroup a base class, with subclasses that pass in the desired icons nodes. That would probably be preferable to complicating the ZoomButtonGroup API.

[pixelzoom]

Another approach would be to make ZoomButtonGroup a base class, with subclasses that pass in the desired icons nodes.

class ZoomButtonGroup extends LayoutBox {
   constructor( zoomInIcon, zoomOutIcon, zoomLevelProperty, options ) {
      ...
   }
}

class PlusMinusZoomButtonGroup extends ZoomButtonGroup {
    constructor( zoomLevelProperty, options ) {

      options = merge( {
         textOptions: {...}
         ...
      }, options );

      const zoomInIcon = new RectangularPushButton( {
        new Text( MathSymbols.PLUS, options.textOptions )
      } );

      const zoomOutIcon = new RectangularPushButton( {
        new Text( MathSymbols.MINUS, options .textOptions )
      } );

      super( zoomInIcon, zoomOutIcon, zoomLevelProperty, options );
    }
}

class MagnifyingGlassZoomButtonGroup extends ZoomButtonGroup {
    constructor( zoomLevelProperty, options ) {

      options = merge( {
         fontawesomeNodeOptions: {...}
         ...
      }, options );

      const zoomInIcon = new FontAwesomeNode( 'search-plus-solid', options.fontawesomeNodeOptions );
      
      const zoomOutIcon = new FontAwesomeNode( 'search-minus-solid', options.fontawesomeNodeOptions );

      super( zoomInIcon, zoomOutIcon, zoomLevelProperty, options );
    }
}

This approach also supports the "future", when PhET needs a 4th way to do zoom buttons.

[samreid]

The preceding proposal #620 (comment) looks good to me, let me know if you would like me to implement it.

[pixelzoom]

If you need it now, go for it. You'll need to add the fontawesome glyphs to FontAwesomeNode.js.

EDIT: I'll be happy to review.

[pixelzoom]

Before you begin... I'm still skeptical that any of this is needed. I haven't heard a response to why this is needed for CCK (+/- seems clear to me in the context of the chart), and why it's desirable to have 3 different "looks" for zoom buttons in PhET sims.

[pixelzoom]

In #620 (comment), you said:

Should the chart zoom buttons look identical to the play area zoom buttons? If so, what styling/coloring should we use?

I should also point out that these buttons will not look identical to the play area zoom buttons. They will be using fontawesome icons. And they will not be separated by space. And if you have thoughts about adding that space to ZoomButtonGroup, that's going to complicate the pointer areas, which are currently implemented based on the assumption that the there is no space between the buttons, and the pointer areas therefore need to be shifted.

[samreid]

OK let's wait until we hear from designers. @kathy-phet @arouinfar or @ariel-phet the main questions are:

• Does the CCK chart require magnifying glass icons, or are "+" and "-" OK?
• If so, should the play area buttons and the chart zoom icons have the same look?
• If not, will we have other sims where the zoom buttons will need to show icons other than "+" and "-"?

@pixelzoom please add other questions if I missed anything.

[samreid]

I committed as @pixelzoom and I discussed. @pixelzoom do you have time to review? If not, please reassign to @ariel-phet to request another reviewer.

UPDATE: After review is complete, we can open a chip-away issue to migrate from ZoomButton to ZoomButtonGroup.

[samreid]

I added one more commit to use the magnifying glasses in the CCK chart.

[pixelzoom]

A bit of cleanup needed. I addressed these things in commit phetsims/scenery-phet@7e7e5bd:

• In MagnifyingGlassZoomButtonGroup.js, this assertion is needed, as you're potentially (silently) blowing away an icon specified by the client:

assert && assert( !options.magnifyingGlassNodeOptions.icon, 'MagnifyingGlassZoomButtonGroup sets magnifyingGlassNodeOptions.icon' );

• In PlusMinusZoomButtonGroup.js, this constant isn't going to make sense to future readers. Size of what? Why the funky computations involving magic numbers?

const DEFAULT_SIZE = new Dimension2( 20 * 0.35, 3.6 * 0.35 );

• In PlusMinusZoomButtonGroup.js, this line is unnecessary. You've already set size: DEFAULT_SIZE above, so you can just pass options.iconOptions to the icons.

const plusMinusNodeOptions = merge( { size: DEFAULT_SIZE }, options.iconOptions );

Back to @samreid for review of these changes. Then feel free to close.

[samreid]

I reviewed the changes and did not have further recommendations, thanks! Closing.