search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Should the CloseButton use generalCloseSoundPlayer by default? #859

Closed jessegreenberg closed 3 weeks ago

jessegreenberg commented 1 month ago

This option is passed to the CloseButton, should it use this soundPlayer by default?

      soundPlayer: generalCloseSoundPlayer,

For phetsims/mean-share-and-balance#270

marlitas commented 3 weeks ago

I saw this comment in dialog, so while it wouldn't hurt it is definitely not needed for it's largest use case which is Dialog. 

 // turn off default sound generation, Dialog will create its own sounds
      soundPlayer: nullSoundPlayer,

The closeButton is currently only being used in Dialog and older javascript sims. The default styling of it also does not seem like it is up to date with our design aesthetic. Neuron Usage:

image

Dialog Usage:

image

Since this is regarding a scenery-phet component I'm going to move this issue there, but I think it is ultimately the decision of the responsible dev for that repo which is @jessegreenberg.

marlitas commented 3 weeks ago

@jessegreenberg I'm happy to add the generalCloseSoundPlayer to CloseButton if you decide we should proceed with that.

jessegreenberg commented 3 weeks ago

Thanks @marlitas - I think it should. Ill add it and then remove the usage in mean-share-and-balance (and any others I can find). This will result in a behavior change for cases like Neuron as you have shown, and since it relates to sound Ill see if @jbphet is available for review.

jessegreenberg commented 3 weeks ago

Commits made above. I looked for other usages of CloseButton to see if there were any sound conflicts with custom sounds but didn't notice any.

@jbphet would you mind reviewing, are you OK with adding this default sound to CloseButton?

jbphet commented 3 weeks ago

This definitely seems appropriate. The commits look good, and I test drove it in the Mean Info Dialog in mean-share-and-balance, and it sounds good. Closing.