RectangularRadioButtons should only phet-io instrument their enabledProperty if there are >2 buttons in the group

[samreid]

From discussion in phetsims/circuit-construction-kit-common#946, RectangularRadioButtons should only phet-io instrument their enabledProperty if there are >2 buttons in the group.

I do not believe Beer's Law Lab or Concentration use RectangularRadioButtons, so this probably wouldn't disrupt that dev/rc cycle.

[samreid]

Implemented in the commits. I added the following documentation.

    // The ButtonModel implements the enabledProperty, so we must pass through
    // any customizations to that instrumentation.  In particular, RectangularRadioButtons in a group of 2 or less
    // cannot be disabled, because that would leave just one button enabled.  RectangularRadioButtons should not be
    // used like a legend, and if they cannot be used to make selections, the group should be made invisible
    if ( options.hasOwnProperty( 'phetioEnabledPropertyInstrumented' ) ) {
      buttonModelOptions.phetioEnabledPropertyInstrumented = options.phetioEnabledPropertyInstrumented;
    }

Ready for review. Possibly by @zepumph ? If there's time.

[pixelzoom]

I don't think this is ready for review, it's a half-way solution.

@samreid lets discuss:

• Why does this not also apply to visibleProperty?
• Why does this not also apply to AquaRadioButtonGroup?
• How should this be coordinated with Buttons in ZoomButtonGroup should not have visibleProperty instrumented scenery-phet#796?

[pixelzoom]

I do not believe Beer's Law Lab or Concentration use RectangularRadioButtons, so this probably wouldn't disrupt that dev/rc cycle.

If this is going to be the new standard, then it should also apply to AquaRadioButtonGroup. And BLL/Concentration definitely uses those. It currently uninstruments their visibleProperty, but exposes their enabledProperty. For example:

[samreid]

In discussion with @pixelzoom @arouinfar @zepumph @matthew-blackman and myself, we will uninstrument the visibleProperty and enabledProperty when numberOfButtons<=2 (for aqua + rectangular radio buttons), and will do the same for the zoom button groups.

This will simplify the tree, and make it so designers don't need to iterate through and unfeature things.

[kathy-phet]

@pixelzoom @arouinfar @zepumph @matthew-blackman , @samreid - Just a question about this. If I'm understanding, you won't be able to hide or disable one of the radio buttons when there are 2. One question here ... sometimes these radiobuttons serve as a bit of a legend so you want to keep it visible, but you want the student to stay on one of the 2 specific radio buttons for a while. It sounds like uninstrumenting will mean the instructional designer won't be able to do this? Keep the 2 radio buttons, but disable the one they don't want students to select (or they don't want students to select at the moment).

If the above is accurate description, I would vote for just leaving it instrumented and keep the flexibility here.

[samreid]

@matthew-blackman @arouinfar and I discussed this again today.

@samreid and @matthew-blackman argue that based on @kathy-phet remarks and @pixelzoom remarks from yesterday, that we should simplify things, allow clients to do as they wish and add visibleProperty and enabledProperty for all radio buttions/aqua radio buttons, even if there are only 2. We would like to even go further to simplify our lives and allow all of those to be phetioFeatured: true since we agreed instructional designers may wish to customize these.

@arouinfar feels it would be better to simplify the tree as much as possible, whether through uninstrumenting or unfeaturing things like this, and only opting in on a case by case basis.

Since we are in disagreement, we will follow @kathy-phet recommendation. That is: add visibleProperty and enabledProperty for all radio buttons, even if there are only 2. Featuring will be on by default, but can be overridden.

We would like to discuss issues like this more once @kathy-phet and @pixelzoom are together, but we wanted to have a decision for taking Beers Law Lab shas for today.

In the future, a radio button "display only" property like we have for ComboBox may be appropriate.

[samreid]

In looking at Beer's Law Lab, we see the buttons have enabledProperty, but not visibleProperty. Therefore we need to reinstrument the visibleProperty.

[pixelzoom]

We would like to discuss issues like this more once @kathy-phet and @pixelzoom are together, but we wanted to have a decision for taking Beers Law Lab shas for today.

I'm going to abstain.

[samreid]

OK @matthew-blackman and I implemented what was agreed upon. We believe this unblocks Beer's Law Lab for RC. But this issue should remain open so we can update the Rectangular Radio Buttons accordingly. This likely also impacts the zoom button group.

[pixelzoom]

This likely also impacts the zoom button group.

Why does this impact ZoomButtonGroup? It does not use radio buttons. There's no reason to use it as a "display". There's no reason to hide buttons individually. And the sim controls whether the buttons are enabled.

[pixelzoom]

And while I'm abstaining on whether to instrument specific radio button properties... I will say (again) that I'm not at all a fan of mangling UI components to turn them into "displays". It results in an lousy (embarassing!) UX. And given the time that PhET spends on polishing the UX for sims, it makes no sense to me that you'd let clients abuse the UX in this way. If clients really need "displays", this is absolutely not the way to do it. Put the resources into creating proper "display only" modes for UI components (see https://github.com/phetsims/phet-io/issues/1757), or tell clients they can't do that for now. By allowing them to do it this way, you're creating more unnecessary PhET-iO elements, and a sub-standard way of creating "displays" that PhET may need to support forever.

[samreid]

I reverted the "2 or more" rule. I was going to add back the visibleProperty for the lifelike and schematic radio buttons for CCK, but realized it would be very awkward to show just one battery below the carousel. So I'm going to leave that out for now. I expect we'll talk about this more in upcoming phet-io sims.

[samreid]

To do:

• Check aqua radio button group to see if it matches the same conventions as RectangularRadioButtonGroup (both visibleProperty and enabledProperty instrumented by default)

[samreid]

For the aqua radio button group, the phetioEnabledProperty is instrumented by default, and so is the visibleProperty. This issue seems OK to close. Anyone feel free to reopen if there is more to do or discuss.