Radio button labels are not children of radio buttons

[pixelzoom]

Related to #334 and noticed while working on phetsims/sun#615.

The structure of this radio button group looks very odd. Shouldn't the radio button labels be children of the radio buttons?

[jbphet]

As @samreid noted in today's phet-io meeting, the label on the radio button is the responsibility of the client. RadioButtonGroup creates the tandems for the individual radio buttons based on the information provided to its constructor in the tandemName values of the provided items. It would thus be a bit tricky to have the text values be a sub-item of the radio button. So, while I agree that it would seem a bit more intuitive in the Studio UI if the text were sub-fields of the radio buttons, I didn't know of a practical way to make it happen.

If this we want to consider supporting this sort of a configuration, it's a bigger conversation than what we should do for this sim. I'm going to assume that this is non-blocking for the 1.2 release and leave it to @arouinfar to decide whether this should be discussed at a phet-io meeting.

[pixelzoom]

This is also another one of those cases where using a Property to change text is an expensive/inappropriate approach. It's highly unlikely that the radio button labels need to be dynamically changed after the sim is running. If we had a means of overriding string values, then we wouldn't even need these *Text elements.

[pixelzoom]

I also see that it's not possible to simply change (for example) "neon" to something else in one place. There are 3 different neonText.textProperty elements in this sim, once per screen. If someone wants to change "neon" to something else, they need to do it in 3 places (...another drawback of using Property instead of an API that lets you substitute strings.)

The way I addressed this in pH Scale is to have strings like this appears under "globals", i.e.:

Each solute has a {StringProperty} nameProperty. Change the name there and it changes everywhere.

You could have done something similar in SOM, like:

- globals
  - model
    - atomsAndMolecules
      - argon
        - nameProperty
      - neon
      - oxygen
      - water

Then there would be no need for the odd radioButtonGroup structure shown in #336 (comment), and no need to change the same string in 3 places.

[arouinfar]

Thanks for opening this issue @pixelzoom. I think it generally makes sense for the textProperty to be a child of the radio button or to instead use global where possible, but I don't know how feasible that is for SOM.

The SOM suite also include Atomic Interactions which has a more complicated interface for selecting pairs of atoms. The Interaction screen of SOM is derived from Atomic Interactions, and there are likely complications with how the strings behave.

@jbphet I do not think this should block the 1.2 release, but we should discuss a general pattern at phet-io meeting.

[samreid]

From Aug 20 PhET-iO meeting:

@jbphet: It seemed like the client just wanted to change the text label in one place, not changing it systemically throughout the sim.

@pixelzoom: Let's reach out to the client to see if they want to change things globally/just in certain views and dynamically/statically.

@kathy-phet for now I recommend to leave it as it is, even if it doesn't take too long.

@zepumph: Here's an example of the current recommended pattern: https://github.com/phetsims/inverse-square-law-common/blob/5eaa7a04ba6c60e4535efc49b1b44352366572eb/js/view/ISLCForceValuesDisplayControl.js#L53-L78
I don't recommend using a new pattern for this.

@kathy-phet We can strive to attain that pattern, but perhaps after the deadline.

@jbphet said that example looks like what he was thinking.

@pixelzoom: If you are creating a radio button group, create a subclass like /natural-selection/js/common/view/GraphChoiceRadioButtonGroup.js

But it would need more changes to pass the tandem through like:

      // Population
      {
        value: GraphChoice.POPULATION,
        node: new Text( naturalSelectionStrings.population, merge( {
          tandem: options.tandem.createTandem( 'populationRadioButton' ).createTandem( 'textLabel' )
        }, TEXT_OPTIONS ) ),
        tandemName: 'populationRadioButton'
      },

[jbphet]

Unassigning until the maintenance release is designated as one of my priorities.