phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

RectangularRadioButtonItem.tandem is ignored. #795

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Related to https://github.com/phetsims/sun/issues/787 ...

RectangularRadioButtonItem has the same problem as reported in https://github.com/phetsims/sun/issues/793 - RectangularRadioButtonItem.tandem is ignored.

pixelzoom commented 1 year ago

I suspect that the fix will be similar to the fix for #793 in https://github.com/phetsims/sun/commit/c094a991db939093aa6d181e4508601c1a8e803d. I don't have time to test it, but here's a patch:

Index: js/buttons/RectangularRadioButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/RectangularRadioButtonGroup.ts b/js/buttons/RectangularRadioButtonGroup.ts
--- a/js/buttons/RectangularRadioButtonGroup.ts (revision 5d3b69863689c1eb6bb7aefddf63a25382b2cc64)
+++ b/js/buttons/RectangularRadioButtonGroup.ts (date 1664484513688)
@@ -194,7 +194,10 @@
         minHeight: tallestContentHeight + 2 * yMargin,
         soundPlayer: options.soundPlayers ? options.soundPlayers[ i ] :
                      multiSelectionSoundPlayerFactory.getSelectionSoundPlayer( i ),
-        tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : Tandem.OPT_OUT,
+        tandem: item.tandem ? item.tandem :
+                item.tandemName ? options.tandem.createTandem( item.tandemName ) :
+                options.tandem === Tandem.OPT_OUT ? Tandem.OPT_OUT :
+                Tandem.REQUIRED,
         phetioDocumentation: item.phetioDocumentation || ''
       }, options.radioButtonOptions );
samreid commented 1 year ago

I think the patch looks correct. I committed it over 2 commits since I wasn't sure item.tandem was necessary in the first commit. But since tandem is supported as an option, I realized it is necessary.

In https://github.com/phetsims/sun/issues/787#issuecomment-1261120682 once we eliminate tandem as an option, we can simplify this part. I'll make a note in #787.

Ready for review.

pixelzoom commented 1 year ago

👍🏻 Agreed, tandem will be made unnecessary here once #787 is addressed. Thanks for working on this. Closing.