phetsims / ohms-law

"Ohm's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/ohms-law
GNU General Public License v3.0
5 stars 6 forks source link

Reset Alert over-ridden by slider alerts #81

Closed terracoda closed 7 years ago

terracoda commented 7 years ago

In #74, @terracoda said:

The reset alert is gets over-ridden by the slider alerts. This happens after sliders have changed, and then the reset button is pressed.

Alert for Reset:

jessegreenberg commented 7 years ago

Thanks @terracoda. ResetAllButton needs to use scenery-phet/UtteranceQueue now that it is replacing scenery-phet/AriaHerald. That should solve the problem of overriding alerts.

jessegreenberg commented 7 years ago

@zepumph would you have time to work on this?

zepumph commented 7 years ago

Yeah!

zepumph commented 7 years ago

@jessegreenberg please review. I was not sure what the UtteranceQueue equilelent of AriaHerald.enabled was, so to enable it again I am clearing the queue, and then unmuting it to "start over" after a reset. What do you think?

jessegreenberg commented 7 years ago

I agree that seems like the best way to do it with the current API.

Would it be useful to have a way to disable the UtteranceQueue like we could with AriaHerald?

If disabled, we no-op in addToFront, addToBack and next?

zepumph commented 7 years ago

That would save us a step and be a bit more explicit. Current api:

I'm not sure what the use case is, or if there is one, but it backs up the idea that we should have a "disable" "enable" feature set for Utterance Queue as well.

jessegreenberg commented 7 years ago

Then it seems like it might also be useful to pause the queue? Apart from disable and enable, to support that case we might want to

zepumph commented 7 years ago

Maybe when the UtteranceQueue is paused, things can still be added, but when disabled it's a no-op

jessegreenberg commented 7 years ago

Yes, that sounds like a good way to categorize it.

zepumph commented 7 years ago

I added enabled disabled, and I think it works really well for the reset alert in this issue. @jessegreenberg please review. Should we implement pause as well?

jessegreenberg commented 7 years ago

Looks great @zepumph, but I am wondering why you chose to no-op clear when the queue is disabled? If the queue is disabled we might still want to quickly remove all utterances?

jessegreenberg commented 7 years ago

But I verified that ResetAllButton alerts are working correctly!

zepumph commented 7 years ago

I agree. clear is no longer a noop (spoken like 'nuup'). Closing