phetsims / sun

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

Have checkbox speak context responses #734

Closed jessegreenberg closed 2 years ago

jessegreenberg commented 2 years ago

While working on greenhouse effect description @jbphet recommended that the context response associated with the checkbox go through input with that UI component instead of linking to the Property. This simplifies simulation code, and also makes it clear that content will be spoken from user input and not when the Property changes from a ResetAllButton press (for example).

We added a checkedContextResponseUtterance and a uncheckedContextResponseUtterance to checkbox. They are spoken with alertDescriptionUtterance in the Checkbox toggleAction. I decided to make the option of type Utterance so that the alert content can easily be updated with a reference to the Utterance in the simulation.

It is working well for us, but I thought @zepumph should review this change. Do you have any thoughts on doing this kind of think in a UI component like Checkbox? They are being used now in InstrumentVisibilityControls in greenhouse-effect.

zepumph commented 2 years ago

I decided to make the option of type Utterance so that the alert content can easily be updated with a reference to the Utterance in the simulation.

I don't really care either way, but this seems a bit weird to me, why not just pass in the string? Who cares, it's common code?

https://github.com/phetsims/greenhouse-effect/blob/ec611bd59bf5ba6a0142d4f94d38b9ca268b2723/js/common/view/InstrumentVisibilityControls.ts#L76

Thanks for doing this.

jessegreenberg commented 2 years ago

Ah, right, thanks - I updated it to take an AlertabldDef (updating phet-types.d.ts) and renamed the option name accordingly. Then I simplified usages.

zepumph commented 2 years ago

Sounds Great Thanks