RadioButton and ComboBoxItem tandem name convention

[zepumph]

Current convention: gravityRadioButtonGroup.onRadioButton
Potential proposal: gravityRadioButtonGroup.on

In SOM we have names like: statesOfMatter.statesScreen.view.moleculesControlPanel.radioButtonGroup.neon.enabledProperty
In most other places (like timeControlNode): gravityAndOrbits.modelScreen.view.timeControlNode.speedRadioButtonGroup.normalRadioButton

Discussion from slack:

Amy Rouinfar:spiral_calendar_pad: 12:07
Please open an issue @KatieWoe.

Chris Malley 12:07
radioButtonGroup.neon is not what we usually do?

Kathryn Woessner:house_with_garden: 12:07
phetsims/states-of-matter#331

Chris Malley 12:08
For example, naming of radio buttons in NS:
naturalSelection.introScreen.view.environmentRadioButtonGroup.arcticRadioButton
naturalSelection.introScreen.view.environmentRadioButtonGroup.equatorRadioButton

Amy Rouinfar:spiral_calendar_pad: 12:08
I think we need a general iO design pattern for RadioButtonGroup. There is variation in how I’ve seen them in studio.

Chris Malley 12:08
not:
naturalSelection.introScreen.view.environmentRadioButtonGroup.arctic
naturalSelection.introScreen.view.environmentRadioButtonGroup.equator
12:09
The general pattern for all UI components is to name them based on what they are.
12:09
RadioButtonGroup contains RadioButtons

Amy Rouinfar:spiral_calendar_pad: 12:11
Depending on the situation it may simply be radioButtonGroup in the tree, and I’ve seen the buttons themselves named various things. We’ve tried to clean them up in review, but there are some constraints in the structure of the SOM code (like the text and the button being siblings).

Chris Malley 12:12
Just describing what I understand to be the conventions.
👍:skin-tone-3:
1

12:14
And there's nothing in common code that requires it to be just radioButtonGroup in the tree. There's also nothing in common code that enforces that the tandemName provided for each radio button must end with "RadioButton". Other PhET-iO code enforces a suffix, e.g. PhetioGroup tandem must end with "Group".

Amy Rouinfar:spiral_calendar_pad: 12:17
@Kathy and I have explicitly asked for individual radio buttons to be named radioButtonGroup.title instead of radioButtonGroup.titleRadioButton in several sims because it makes the tree a lot nicer to read. There has never been pushback, so this is news to me. radioButtonGroup.titleRadioButton looked really redundant to us.

Sam Reid 12:18
I’m surprised to hear it, I’ve presumed that radio buttons should have a “RadioButton” suffix
12:18
We should not overoptimize based on just what it looks like in the tree. The tandem names are intended to be used elsewhere

Chris Malley 12:18
News to me too. "neon" doesn't tell you anything about what to expend for child elements, whereas "neonRadioButton" does. And we frequently refer to tandem names out of "full phetioID" context. (edited)

Amy Rouinfar:spiral_calendar_pad: 12:24
I think we saw the radioButtonGroup.title pattern somewhere, liked it, and then asked for it in other places. I’m trying to remember details from 6+ months ago, though.

Kathy Perkins:spiral_calendar_pad: 14:49
What Amy says is my recollection too, ☝️. It was similar to combo box list of choice. Let's discuss tomorrow. (edited)

[zepumph]

@samreid mentioned that it is possible while developing a wrapper to have the componentName without the context of the whole PhET-iO ID.

@arouinfar and @kathy-phet have been designing phetioIDs by largely using the context (thus the example of the SOM radio buttons).

@pixelzoom mentioned that it was confusing when we get rid of the "RadioButton" suffix because it isn't clear what inside of the "RadioButtonGroup".

We decided that group elements should have a suffix for clarity. This means:

• RadioButtons get the suffic "RadioButton" (SOM will need to change)
• ComboBox.listBox children get Item as a suffix
• All checkboxes get a Checkbox suffix
• @arouinfar will look through other group-like containers to see if there are any more.

Just like Property, we need to assert that these suffixes are present in the common code types.

For this issue:

• create an issue in EFAC
• Add assertions about the above prefixes.
• Update any phet-io instrumentation guide doc
• @jbphet to make an issue to handle SOM specific changes.

[zepumph]

These phetioIDs need to be spick and span for publication.

[pixelzoom]

I'll take care of enforcement of the "Item" suffix in ComboBoxItem.

[zepumph]

When doing these renames, don't forget about how they may effect overrides files, and client guides.

[pixelzoom]

I don't see that anyone has volunteered for making similar changes for RadioButtonGroup. @zepumph shall I do that?

[zepumph]

You are welcome to, thanks @pixelzoom!

[pixelzoom]

I've completed changes for ComboBoxItem. It now requires tandemName to have an "Item" suffix.

Sims that were affected:

beers-law-lab @pixelzoom
cck-black-box @samreid
molarity @zepumph and
ph-scale @pixelzoom
projectile-motion @jbphet

@jbphet if you want to cherry-pick the ComboBoxItem changes to SOM, the shas are 187ac98 and 55c4fc0

[kathy-phet]

Assigning @jbphet

[pixelzoom]

All work here is completed.

@ariel-phet please assign someone to review.

[pixelzoom]

Also assigning to @arouinfar, because I did fix up the GAO and SOM client guides.

[zepumph]

I'll review!

[zepumph]

I looked through every commit above. I searched through every usage of tandemName in the project looking for other cases. I also did phetsims/axon#322. Fuzzing is clear.

I was surprised that no overrides files needed changing, but that would fail quite loudly if one phetioID rename was missed in there.

I don't feel like I have a good idea if the changes in client guides are complete (and I'm looking forward to https://github.com/phetsims/phet-io-sim-specific/issues/21!). Over to @arouinfar for anything else for the client guides. and @jbphet if he wants to cherrypick commits into branches as offered in #615 (comment).

I don't think this needs to block publication from a code-perspective anymore, but I think it will be good to have a designer give it a once over before unlabeling it.

[arouinfar]

Also assigning to @arouinfar, because I did fix up the GAO and SOM client guides.

Thanks @pixelzoom. The guides are looking good. The SOMB guide also needed updating, which I did and referenced in a commit above.

I was surprised that no overrides files needed changing, but that would fail quite loudly if one phetioID rename was missed in there.

This also surprised me at first. The phetioFeatured behavior for these components is handled in common code, not sim-by-sim in overrides.

I don't feel like I have a good idea if the changes in client guides are complete (and I'm looking forward to phetsims/phet-io-sim-specific#21!). Over to @arouinfar for anything else for the client guides.

All the guides are looking good to me.

[arouinfar]

These changes affect the EFAC rc test, phetsims/qa#528.

[jbphet]

The changes have been propagated to the current release branch for SOM (1.2). I'm thinking this is pretty much done, but I'll send it to @chrisklus since the EFAC RC test was mentioned to make sure that it's covered there.

[chrisklus]

The EFAC RC is going to be rebuilt since it hasn't been started by QA yet and there's been so many common code changes. Closing.