itaditya / trick-or-treat-game

Play it for free here
https://trick-or-treat.netlify.com/
16 stars 17 forks source link

Added background audio #21

Closed macroexpansion closed 5 years ago

macroexpansion commented 5 years ago

17

itaditya commented 5 years ago

Instead of directly loading BackgroundAudio component, can you lazy load it with import() and Suspense?

This is a good article https://alligator.io/react/code-splitting-with-react-suspense/

macroexpansion commented 5 years ago

ok thanks. i'm doing it

itaditya commented 5 years ago

There is a gotcha with latest browsers, they disable autoplay audio by default.

The way to go is that

  1. Keep the audio muted <audio muted />
  2. When user interacts with page then unmute the audio.
itaditya commented 5 years ago
Screenshot 2019-10-02 at 12 10 02 AM
itaditya commented 5 years ago

@kemda26 your approach is interesting but it has some limitations.

  1. It doesn't play music when user presses a key. Since the primary interaction on desktop for this game is a keyboard rather than mouse it is very important. Though using a onKeydown will work here.

  2. You are using React state at the top level which would cause the whole App to re-render plus we are sharing the concerns of playing audio to the top-level.

I propose a simpler solution which doesn't require state or any code changes outside BackgroundAudio component.

In the useEffect of BackgroundAudio, add document event listeners for click and keydown like this

document.addEventListener('click', playAudio);
document.addEventListener('keydown', playAudio);

The playAudio function would just use the ref of audio DOM elem and play it. I just found out that there is no need to mute and unmute also. If the audioElem.play() happens after a user interaction browser doesn't block it. So basically the autoplay was the main culprit.

In your code I noticed you are not using useRef for ref with audio elem, I think it's better than the callback way you are using. Can you make all these changes?

You are doing great! This would be a very valuable contribution to the game. Thanks for spending so much time in this.

macroexpansion commented 5 years ago

ok. i’ll try it. i kinda new to react. thanks for helping me :)

itaditya commented 5 years ago

Oh seriously! the work you did was so awesome I assumed you're a PRO in React.

One thing I noticed was the mp3 file is 3MB. Can you use a online file compressing software also to reduce the file size. Ideally the file size should be 700KB.

netlify[bot] commented 5 years ago

You can play this version of the game here

Built with commit 9e9e7b28fe6ecd9dc2a4cdf20e9262b9e8d3c62c

https://deploy-preview-21--trick-or-treat.netlify.com

macroexpansion commented 5 years ago

there's a problem with Firefox is that it doesn't recognize arrow keys or any non-printable keys (CTRL, ALT...) as interacting with browser. so the audio can't be played. but it works fine with Chrome I think.

itaditya commented 5 years ago

I tried my solution locally, it works in Firefox and Chrome

itaditya commented 5 years ago

@kemda26 any progress? If you got stuck, can you commit your code though so I can see how you are implementing it now and help you. I really want to get background sounds in the game. It would be so cool.

macroexpansion commented 5 years ago

the code in the latest commit. it doesn’t work with arrow keys in Firefox

itaditya commented 5 years ago

@kemda26 everything works smoothly for me on Firefox and Chrome and so I want to merge this PR. Can you resolve the conflicts. If not, enable the Allow Edits from Maintainers checkbox.

Read more here

macroexpansion commented 5 years ago

oh ok. i fixed conflicst and also turned on Allow edits from maintainers.

itaditya commented 5 years ago

@kemda26 you have not run npm run format on the code, can you do that?

macroexpansion commented 5 years ago

ok i ran it

itaditya commented 5 years ago

Congrats on getting the PR merged @kemda26. This was quite a feat since this PR involved working around browsers, lazy loading with Suspense.

Congrats