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

Add voicing to Checkbox #742

Closed
zepumph opened this issue Feb 23, 2022 · 7 comments
Closed

Add voicing to Checkbox #742

zepumph opened this issue Feb 23, 2022 · 7 comments

Comments

@zepumph
Copy link
Member

zepumph commented Feb 23, 2022

From phetsims/ratio-and-proportion#363. This is pretty straight forward because of @jessegreenberg's work in 6afc6a9.

zepumph added a commit that referenced this issue Feb 24, 2022
@zepumph
Copy link
Member Author

zepumph commented Feb 24, 2022

@jessegreenberg
Copy link
Contributor

Looks really nice, and works well. Thanks @zepumph.

@zepumph
Copy link
Member Author

zepumph commented Mar 7, 2022

@pixelzoom just found that the typing is wildly wrong for voicing. The checkedContextResponse is TAlertableDef for description, but basically just a string for voicing.

@zepumph
Copy link
Member Author

zepumph commented Mar 7, 2022

It isn't clear to me how best to proceed with this. The easiest way would be to have each accept the same type, but there isn't really that support at this time, so I'm not really sure. I'm also quite surprised that we haven't run into this in any other common code just yet.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Mar 7, 2022

Maybe these options should take the subset of types that both Voicing and PDOM support, that would basically just be string or null. Or the Voicing implementation in Checkbox can turn the TAlertableDef into a VoicingResponse. Maybe we have a utility function to convert a TAlertableDef into a VoicingResponse.

pixelzoom added a commit that referenced this issue Mar 7, 2022
@pixelzoom
Copy link
Contributor

Here are the TODOs in CheckBox that need to be addressed:

      if ( property.value ) {
        options.checkedSoundPlayer.play();
        options.checkedContextResponse && this.alertDescriptionUtterance( options.checkedContextResponse );
        // @ts-ignore TODO https://github.com/phetsims/sun/issues/742
        this.voicingSpeakNameResponse( { contextResponse: options.checkedContextResponse } );
      }
      else {
        options.uncheckedSoundPlayer.play();
        options.uncheckedContextResponse && this.alertDescriptionUtterance( options.uncheckedContextResponse );
        // @ts-ignore TODO https://github.com/phetsims/sun/issues/742
        this.voicingSpeakNameResponse( { contextResponse: options.uncheckedContextResponse } );
      }

@zepumph
Copy link
Member Author

zepumph commented Mar 12, 2022

Typing here has been solved over in phetsims/utterance-queue#67. See e218a68

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