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

Should the CloseButton use generalCloseSoundPlayer by default? #859

Closed
jessegreenberg opened this issue Jun 13, 2024 · 5 comments
Closed

Comments

@jessegreenberg
Copy link
Contributor

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

      soundPlayer: generalCloseSoundPlayer,

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

@marlitas
Copy link
Contributor

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 marlitas assigned jessegreenberg and unassigned jbphet and marlitas Jun 26, 2024
@marlitas
Copy link
Contributor

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

@marlitas marlitas transferred this issue from phetsims/mean-share-and-balance Jun 26, 2024
@jessegreenberg
Copy link
Contributor Author

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

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

jbphet commented Jun 26, 2024

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.

@jbphet jbphet closed this as completed Jun 26, 2024
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