Drum Machine: Request for Review

Code Feedback
1

I’m happy to have completed the Drum Machine project. I’ll appreciate your review and feedback. The app is live here and the GitHub repo is here.

2 Likes
2

If this is the certificate project it should include the test script and it must pass all the tests before you submit it.

It is not going to be tested when you submit it you have to test it using the test script and only submit it after it passes all the tests. Your project is not passing the tests.

If you already did the submit, redo it with code that passes all the tests.

You can still have your own version where you do whatever you like but that can’t be used for the certificate project.

Anyway, congrats on creating the app. The most important part of the projects is the learning experience.

1 Like
3

Thank you, @lasjorg, for pointing out that the project doesn’t pass the test. Indeed, it doesn’t. I’m really not building the projects to get the certification. I just make use of the project ideas.

I’ll appreciate it if you can give me feedback on the project. What things did I do right? What aspects don’t follow the established standards and need to be improved?

4

Hey, this is really good, one of the nicer ones I have seen. You even included a volume control, which I usually don’t see on these projects. I’m very impressed with its responsiveness. It even handled 400% zoom! And you even used button elements for the drum pads. Not to be too hard on the average React developer, but they often just use divs for everything :slightly_smiling_face:

I do have a few suggestions for you. Most of these have to do with accessibility since that’s my specialty. I will note that your accessibility is much better than the fCC demo. I should probably volunteer to fix that demo some day :slight_smile:

  • The volume slider needs a label. You can just change the p to a label on the “volume” text and then add the necessary for and id attributes. Any type of functional element always needs to have an accessible name and using a label is the easiest way to do that.

  • “POWER” and “VOLUME” should probably be heading elements. If this were my drum machine, I would have an h1 at the top with the name of my drum machine in it. Then the power and volume would be h2 elements. I would also add an h2 heading for the display. If you didn’t want that heading to actually appear on the page then you could visually hide it. The reason I would add all of these headings is because they are a primary way that people who can’t see the page and are using a screen reader will navigate your page. A good heading structure is probably the most important thing you can add to your page to make it accessible to people using a screen reader.

  • The power button is an interesting case. There is perhaps no one right way to implement something like this. I do think it has the potential to be visually confusing. And definitely it sounds weird when listening to it with a screen reader. But it is also a very common pattern and so I’m sure most people would be able to figure it out very quickly. Personally, I think a toggle switch is a much better representation of a power button. This is what the fCC demo uses (it just isn’t implement correctly behind the scenes). If you want to try a toggle switch then below is an article that will help you create an accessible one. It even builds a power button switch as the example, which is exactly what you are looking for.

ARIA: switch role

2 Likes
5

It’s fine as long as you do not submit it as a certificate project.

As for the code, it looks pretty good. But here are some things to consider.

  1. If you move the playAudioOnKeyDown function definition inside the useEffect it won’t complain about the missing function dependency and you don’t need useCallback. I know it looks a little weird but it is the officially suggested solution to it (react.dev: Removing unnecessary function dependencies). Edit: also you are missing the volume dependency in the useEffect.

  2. Move the isOn check inside the playAudioOnKeyDown function and do an early return instead of wrapping the addEventListener code in a conditional.

  3. You have a small delay in the initial playing of the audio files. If you download the files and have them locally in the public folder it might fix it.

  4. I would consider disabling the buttons when the power is off. It should be fairly easy to conditionally set the disabled attribute on the buttons using isOn but the logic is a bit confusing (because isOn needs to be true to disable the button). It also isn’t great UX that the button animations will still work so you might want to disable that as well then the buttons are disabled.

1 Like
6

This is detailed! Thank you for your time and attention. I’m very grateful.

1 Like
7

Thank you for this knowledge-packed feedback. I’m very grateful.

1 Like
8

How does the inclusion of a volume control enhance the user experience, particularly in terms of adjusting the audio output of the drum machine?

9

The same way you have an individual volume control for the YouTube video you are watching. It’s nice to be able to control the volume of each individual app separately instead of having to rely on the main system control.

10

Individual app volume control, Mzansinow.co.za/ to YouTube videos, provides convenience, allowing independent adjustments for a tailored audio experience