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 a beep sound #70

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ringtailsoftware
Copy link
Contributor

CC0 licensed, no attribution needed, https://opengameart.org/content/8-bit-sound-effect-pack

@benbrown
Copy link
Owner

benbrown commented Jan 3, 2023

Alas, in Chrome, this throws an error instead of playing the sound.

"app.js:45 Uncaught (in promise) DOMException: play() failed because the user didn't interact with the document first. https://goo.gl/xX8pDD"

@benbrown
Copy link
Owner

benbrown commented Jan 3, 2023

Huh! It seems to be working after some small tweaks.

@benbrown
Copy link
Owner

benbrown commented Jan 3, 2023

A few things to do before this merges:

  • put the sound at a normal url like public/beep.mp4 or whatever it is (make it easier for people to change, find, etc)
  • sound currently fires every page load. should only fire when the number changes. tricky.

@ringtailsoftware
Copy link
Contributor Author

Changed to use /audio/beep.wav
AIUI, the beep sound does not play on every page load as app.newDMs will be undefined, https://github.com/benbrown/shuttlecraft/blob/main/public/app.js#L90

@benbrown
Copy link
Owner

benbrown commented Jan 3, 2023

app.newDMs should be initialized to 0, not doing so was a bug. Any of the other counters could also immediately return with a number and cause the beep to fire, and as implemented could cause multi-beeps!

Just thinking out loud here but I think to get this right I need to:

  • improve the implementation and generalize the availability of the notification indicators
  • perhaps render them serverside and initialize with real current values
  • make the client side code a bit more sophisticated about when to beep

@benbrown benbrown added the wont-merge indicates this PR will not be merged label Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wont-merge indicates this PR will not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants