search

phetsims / sun

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

Option `voicingHintResponse` does nothing in RectangularRadioButtonGroup #773

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

While working on https://github.com/phetsims/sun/issues/740 (cleanup of RectangularRadioButtonGroup), I noticed this option in SelfOptions:

  // voicing - hint response added to the focus response, and nowhere else.
  voicingHintResponse?: VoicingResponse;

Problems:

(1) This option is defined in VoicingOptions, so has no business being in SelfOptions for RectangularRadioButtonGroup.

(2) voicingHintResponse is not used in the implementation of RectangularRadioButtonGroup, and is propagated to super.

(3) Super is doing nothing with voicingHintResponse. RectangularRadioButtonGroup extends FlowBox, so Voicing (and VoicingOptions) is not part of the heirarchy. Attempting to remove voicingHintResponse verifies that it's not being inherited from FlowBoxOptions.

pixelzoom commented 2 years ago

In the above commits, I used an assertion to test whether any caller was using the voicingHintResponse option. I found zero uses, and the default is voicingHintResponse: null. So I went ahead and remove voicingHintResponse as an option to RectangularRadioButtonGroup.

Recommended to review this with high priority, in case I've misunderstood something here.

jessegreenberg commented 2 years ago

I think your change is correct and this can be closed. But I found the history in https://github.com/phetsims/sun/issues/699 so @zepumph can you please take a look?

My understanding is voicingHintResponse was added to defaultOptions initially when many defaultOptions were given to each RectangularRadioButton with https://github.com/phetsims/sun/commit/6ab0a64a7935711c3e18b451d7edd98c02485e5d#diff-b7e10c73fda4b63e04561052358ebf1050cda9ae954b19d443a4c8806dc0694bR177-R178

But now that we have nested radioButtonOptions from https://github.com/phetsims/sun/issues/740, the way to accomplish this would be to provide voicingHintResponse to those. But currently I didn't see any usages of passing a voicingHintResponse to a RectangularRadioButtonGroup.

If that is correct I think this can be closed.

zepumph commented 2 years ago

Yes, totally. Thanks for cleaning up. We can still provide a hint easily through radioButtonOptions because that is a ButtonNode (which includes VoicingOptions already). Thanks