Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option voicingHintResponse does nothing in RectangularRadioButtonGroup #773

Closed
pixelzoom opened this issue Jul 6, 2022 · 3 comments
Closed

Comments

@pixelzoom
Copy link
Contributor

While working on #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
Copy link
Contributor Author

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
Copy link
Contributor

jessegreenberg commented Jul 7, 2022

I think your change is correct and this can be closed. But I found the history in #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
6ab0a64#diff-b7e10c73fda4b63e04561052358ebf1050cda9ae954b19d443a4c8806dc0694bR177-R178

But now that we have nested radioButtonOptions from #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.

@jessegreenberg jessegreenberg removed their assignment Jul 7, 2022
@zepumph
Copy link
Member

zepumph commented Jul 7, 2022

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

@zepumph zepumph closed this as completed Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants