how to instrument AccordionBox's expandCollapseButton?

pixelzoom commented 5 years ago

In https://github.com/phetsims/graphing-quadratics/issues/112, we want to "feature" graphingQuadratics.*.view.equationAccordionBox.expandCollapseButton.visibleProperty. That is currently not possible, since expandCollapseButton is private to AccordionBox.

Two options:

(1) Set expandCollapseButton.visibleProperty to "featured" inside of AccordionBox. It will be that way for all sims, and will not be customizable. This will take 10 minutes.

(2) Add option expandCollapseButtonOptions (nested options) to AccordionBox. This will allow use to pass "featured" info to expandCollapseButton. But it will also be a big job. There are currently 8 options that are specific to expandCollapseButton, and they will need to be nested into expandCollapseButtonOptions. And there are at least 39 uses of AccordionBox that we'll need to manually inspect and modify. This will take many hours.

Option (2) feels like the right way to go, because there may be other aspects of expandCollapseButton that we want to instrument or customize.

@amanda-phet @ariel-phet @kathy-phet your opinions?

@zepumph @samreid FYI.

pixelzoom commented 5 years ago

Here's how featuring expandCollapseButton.visibleProperty would be done if we choose option (2) above.

const accordionBox = new AccordionBox( content, {
  ...
  expandCollapseButtonOptions: {
    ...
    phetioComponentOptions: { visibleProperty: { phetioFeatured: true }  } 
  }
} );

pixelzoom commented 5 years ago

https://github.com/phetsims/sun/issues/477 also affects how we do this. If we proceed with #477, then one of the 2 options above will work. If not, then we will need to reevaluate how to proceed.

amanda-phet commented 5 years ago

I don't think I have enough of an understanding to provide an opinion on which way to go. If I understand option (1) correctly, it sounds like this would always be featured for every accordion box in every sim, and that doesn't seem like a good idea. We might not always want that button to be in the featured list.

pixelzoom commented 5 years ago

@amanda-phet You understand correctly. Option (1) means that expandCollapseButton.visibleProperty would be featured for all sims, with no ability to customize. I agree that's not a good general solution. But someone else will need to make the call on cost vs flexibility.

pixelzoom commented 5 years ago

If we proceed with option (2), here are the AccordionBox options that will need to be nested in expandCollapseButtonOptions. In addition to the internal changes to AccordionBox, uses of these options will need to be identified in all sims, and manually nested.

      // expand/collapse button
      buttonLength: 16, // button is a square, this is the length of one side
      buttonAlign: 'left',  // {string} button alignment, 'left'|'right'
      buttonXMargin: 4, // horizontal space between button and left|right edge of box
      buttonYMargin: 2, // vertical space between button and top edge of box
      buttonTouchAreaXDilation: 0,
      buttonTouchAreaYDilation: 0,
      buttonMouseAreaXDilation: 0,
      buttonMouseAreaYDilation: 0,

Number of occurrences based on string search, may not all be related to AccordionBox:

buttonLength (15) buttonAlign (27) buttonXMargin (42) buttonYMargin (38) buttonTouchAreaXDilation (40) buttonTouchAreaYDilation (43) buttonMouseAreaXDilation (11) buttonMouseAreaYDilation (12)

pixelzoom commented 5 years ago

Hmm.... Looking at these options, there's only one (buttonLength) that can be propagated to ExpandCollapseButton constructor. buttonAlign, buttonXMargin and buttonYMargin are used in layout. *Dilation are used to set pointer areas after the ExpandCollapseButton has been instantiated. So it's probably not appropriate to nest all of these.

EDIT: Correction, the *Dilation options can be renamed and passed through. ExpandCollapseButton superclass RectangularButtonView has options touchAreaXDilation, touchAreaYDilation, mouseAreaXDilation, mouseAreaYDilation.

pixelzoom commented 5 years ago

Here's what I'm considering. @samreid @zepumph please comment.

AccordionBox:

options = _.extend( {
...
  // {*|null} options passed to ExpandCollapseButton constructor, defaults filled in below
  expandCollapseButtonOptions: null,

  // expand/collapse button layout
  buttonAlign: 'left',  // {string} button alignment, 'left'|'right'
  buttonXMargin: 4, // horizontal space between button and left|right edge of box
  buttonYMargin: 2, // vertical space between button and top edge of box
...
}, options );

// fill in defaults
options.expandCollapaseButtonOptions = _.extend( {
  sideLength: 16, // button is a square, this is the length of one side
  cursor: options.cursor,
  tandem: options.tandem.createTandem( 'expandCollapseButton' )
}, options.expandCollapaseButtonOptions );

Client who wants to feature expandCollapseButton.visibleProperty:

const accordionBox = new AccordionBox( ..., {
  ...
  // customize ExpandCollapseButton
  expandCollapseButtonOptions: {
    sideLength: 20,
    touchAreaXDilation: 10,
    touchAreaYDilation: 10,
    mouseAreaXDilation: 5,
    mouseAreaYDilation: 5,

    // phet-io
    phetioComponentOptions: { visibleProperty: { phetioFeatured: true } }  
  }  
} );

pixelzoom commented 5 years ago

Here's a summary of the options that would be nested, and what their new names would be:

current name	nested name	occurrences
buttonLength	sideLength	15
buttonTouchAreaXDilation	touchAreaXDilation	40
buttonTouchAreaYDilation	touchAreaYDilation	43
buttonMouseAreaXDilation	mouseAreaXDilation	11
buttonMouseAreaYDilation	mouseAreaYDilation	12

pixelzoom commented 5 years ago

Ah, rats. ExpandCollapseButton is not a sun button. It's a subtype of Node.

EDIT: As a workaround, I temporarily added dilation options to ExpandCollapseButton, to make its API look like a sun button. See #483.

pixelzoom commented 5 years ago

I'm proceeding with the proposal in https://github.com/phetsims/sun/issues/480#issuecomment-469418368. Affects sims listed below. Run each of these with ?showPointerAreas and compare button size and pointer areas to published version.

[x] area-model suite
[x] balancing-chemical-equations
[x] build-an-atom
[x] curve-fitting
[x] equality-explorer
[x] expression-exchange
[x] fluid-pressure-and-flow
[x] fractions suite
[x] gas-properties
[x] gene-expression
[x] graphing-lines
[x] graphing-quadratics
[x] isotopes-and-atomic-mass
[x] least-squares-regression
[x] models-of-the-hydrogen-atom
[x] plinko-probability
[x] reactants-products-and-leftovers
[x] states-of-matter
[x] trig-tour
[x] unit-rates

samreid commented 5 years ago

The proposal in https://github.com/phetsims/sun/issues/480#issuecomment-469418368 looks reasonable. I do not feel like phetioComponentOptions is an optimal pattern but that will be addressed elsewhere, if at all.

samreid commented 5 years ago

It would be nice to make sure this is all set for GQIO, I'll label it so.

UPDATE: @pixelzoom can you please report on the status of this issue at your convenience?

zepumph commented 5 years ago

I reviewed the above commits, and looked at the current level of instrumentation implementation in master. Things seem pretty nice to me.

I do not feel like phetioComponentOptions is an optimal pattern but that will be addressed elsewhere, if at all.

Agreed that most of this work above seems to have been implemented to support phetioComponentOptions. We now know that for the most part that is an antipattern and we will just be using Studio to do this (BTW I see no usages in GQ, Wahoo!). That said all the above commits look like they are only solid improvements in the right direction. Good work @pixelzoom. To me after reviewing it looks like there is little left here. But I would like to hear from @pixelzoom.

pixelzoom commented 5 years ago

Looks to me like everything is done here, and the answer to the original question is "feature stuff using Studio". So closing.

phetsims / sun

how to instrument AccordionBox's expandCollapseButton? #480