React Drum Machine, seeking feedback

Hello, everyone!

I would appreciate some thoughts on my React Drum Machine code.
Here’s my pen: https://codepen.io/danijels/full/vYyNNQL

Thanks in advance!

2 Likes

Overall, this is a very nice job! I’m going to make a few suggestions just to give you some things to think about :slight_smile:

  • The drumpad is keyboard accessible in that I can press one of the lettered keys and it will play the sound. But I can’t turn the power off with the keyboard (not a show stopper) and I can’t change sound banks with the keyboard (this one is a bit of a bummer). The primary cause of this is because instead of using an <input> you are just adding an event handler to a <div>. There are ways to make toggle switches like this accessible, use your favorite search engine to search for ‘accessible toggle switch’. In general, you should always test whatever you are building with the keyboard only to make sure you can access all functionality.
  • Regarding the headings, you should always start with an h1, which in this case you could add a name for your drum machine to the top of the page. Then each of the direct subheadings below that would be h2 (e.g Power, Bank). I don’t think I’d make the .sound-info displays a heading. Headings act as navigational aids for screen readers and thus they shouldn’t really change (as that would be confusing). Actually, I don’t think you need any headings other than an h1 at the top of the page (see the next point).
  • The volume <input> (and eventually the power and bank <input>s when you make them accessible) need <label>s. That’s easy, just convert the Power and Bank headings to labels and then add a label for volume.
  • I’m getting a horizontal scroll bar before the break point converts the layout to a single column layout. Instead of adjusting the px value on the break point I’m going to recommend a different unit. One thing you’ve done nicely is used em units for most of the element widths. I would recommend you take that one step further and use em for the CSS break point as well. This has the added advantage of making your layout responsive to increases in text size as well. It’s pretty easy to do. You can start by dividing the px value you have now by 16. That will get you close. Then just keep adjusting it a little higher until you no longer get the horizontal scroll bar before the transition. It took me two tries before I got it working for your layout.
1 Like

Thank you so much for your extensive feedback! I’ll definitely look into making some changes.