React hooks Drum Machine

Hey everyone!

I completed the Drum Machine project using React.
Link
GitHub
Looking for feedback here.

1 Like

Can’t really comment on the code without seeing it. Here are a few issues I found.

  1. The mouse click for the drum pads doesn’t seem to work (which is one of the requirements).

  2. The elements are too transparent in my opinion. The yellow color is also not super great with a sepia background color. Maybe increase the blur and lower the sepia filters and/or give the image a color overlay (anything to help with the contrast basically).

  3. It’s not responsive.

  4. I would center the h1 text.

I do like the active styles for the drum pads though.

Hey @lasjorg

Thank you for replying.

  1. I wasn’t aware that clickable drum pads were a requirement, I thought the pads were only to be linked to the keys Q,W,E,A,S,D,Z,X,C. But I’ve updated it this time.

  2. Here’s the Link to GitHub

Can you please comment on the code.

Either you made the repo private or you have deleted it. In any case, we can’t see the code.

Also, I would still suggest you try to improve the contrast a bit more.

Hey @lasjorg,

I made the repo public now. Sorry for the inconvenience.

Hey @kevinSmith, can you please also review this project and its code.

Thanks in advance.

Yeah, it looks pretty good, same comments as above.

Another comment would be that it is not clear to the user what the “bank” toggle is doing. I would expect either some kind of readout for the current bank, or make it a drop-down or radio buttons.

In the code, same comment as last time about the styles.

  const handleClick = (event) => {
    if(!power)  return;
    activeKey.current = event.currentTarget.childNodes[1];
    playSound();
  };

Rather than have early returns from all the handler functions, why not use that flag to disable buttons?

Same comment as before about onClick={(e) => handleClick(e).

I don’t think I like this:

  const handleTransition = (event) => {
    if(!event.target.classList.contains("playing")) return;
    event.target.classList.remove("playing");
  };

This is manually manipulating the DOM, right? That is generally a no-no in React. I would want to use some kind of flag to conditionally apply that class.

onClick={() => toggle()}

Same comment as before, this can probably just be onClick={toggle} - there’s no need to wrap it in a function literal - especially since that could cause unnecessary rerenders.

But still, it looks good. Have fun on the next one.

I like the original styling here but I think you need a little more contrast with the controls. For example, when I turn the power off, the control turns gray and I can barely see it on that background. Yes, there is a white circle there, but I don’t think that’s enough.

Your volume control needs a “Volume” label. I know what that slider is supposed to do because I’m familiar with this project. Others may not. Also, for accessibility, all inputs need labels.

Speaking of the labels you do have, they aren’t doing anything. Yes, they wrap the input, but you’ve removed the input from the page with display: none. Also, even if they wrap the input there still needs to be text associated with the label, which you don’t have.

And since you’ve removed the actual inputs, the controls you do have are not accessible with the keyboard. As a general rule, you should be able to access all functionality with a keyboard only. There are ways to make custom switch inputs and such that are also keyboard accessible. I think you need to do some googling here :slight_smile:

And as @lasjorg mentioned above, this should be responsive. That’s a big one and I would consider this an unsuccessful project until that is fixed.