Drum Machine Feedback - React - Vercel

Hello Everyone!

I would really appreciate any feedback on my Drum Machine
Here are my links:

Specifically here are a couple questions that I have:

  1. I used a click handler, to monitor if a button had been clicked. That is what updates state.name, which in turn updates the display. Should/Could I have done this in the handleButton function which is what is clicking the button
  2. I use a keyboard monitor to handle the button bindings. Then I check against keyCodes that are hard coded into an object. Is that the best way to do that?
  3. I added the event listeners using ComponentDidMount, but then didn’t take them off which is something that I have seen folks do. Why do you need to do this, and how poor form is it to not?
  4. Lastly how does the size/complexity of my components look? Should I have broken things up more/less?

Lastly I figured I would Copy and paste some code here, to make things a bit easier for anyone that is willing to look my work over. Thanks a ton anyone that responds to this. This is actually my first post on FCC, which is kind of exciting. I didn’t copy everything below, but I copied most of the interesting bits.

Here is the parent Component:

class App extends React.Component {
  constructor(props) {
    super(props);

    // Listener Bindings
    this.onKeyPress = this.onKeyPress.bind(this);
    this.onButton = this.onButton.bind(this);

    // Function passed to volume slider to set update the volume state
    this.sliderChange = this.sliderChange.bind(this);

    // state variables. Name will be used in the display window to "name" the sound.
    this.state = {
      name: "Drum Sound",
      volume: 100,
    };
  }
  /* Add the event listeners, one listening for keypress. Will allow for buttons to be triggered by keyboard
    click listener is what will trigger the display update. Will listen for any button click (performed via "click" or 
    key press) and then will update the display*/
  componentDidMount() {
    document.addEventListener("keydown", this.onKeyPress);
    document.addEventListener("click", this.onButton);
  }

  onKeyPress(event) {
    // Monitor key press events. If the keyCode is one of the drumKeys, target that button then click and focus it
    if (drumKeys.hasOwnProperty(event.keyCode)) {
      let button = document.getElementById(drumKeys[event.keyCode])
        .parentElement;
      button.click();
      button.focus();
    }
  }

  onButton(event) {
    // Monitor clicks. If the click occured on a button update the state.name variable (This will update the display)
    if (event.target.type === "submit") {
      this.setState({ name: event.target.id });
    }
  }

  sliderChange = (value) => {
    /* Function passed to the control conponent, updates state.volume which is passed to the drumPad, and then applied to 
    the HTML audio elements when they are played */
    this.setState({ volume: value });
  };

  render() {
    return (
      <div id="drum-machine">
        <DrumPad volume={this.state.volume} />
        <Controls
          name={this.state.name}
          volume={this.state.volume}
          sliderChange={this.sliderChange}
        />
      </div>
    );
  }
}

Here is the DrumPad component. I only included one of the buttons, as they are all the same.

class DrumPad extends React.Component {
  render() {
    /* Declare soundVolume. Passed down from parent. Changed via controls.js Slider Component. */
    let soundVolume = this.props.volume / 100;

    const handleButton = (event) => {
      /* Function to handle a button click (via mouse or keyboard). Method used, grab id of button clicked.
      1. Declare sound to be the HTML audio element of the button. (the first and only child) of the event button
      2. Set the volume property of the audio element. 
      3. Play and enjoy. Maybe dance. It's really up to you. */
      let name = event.target.id;
      let sound = document.getElementById(name).firstChild;
      document.getElementById(name).firstChild.volume = soundVolume;
      sound.play();
    };

    return (
      <div id="drum-buttons">
        <button id="Heater" className="drum-pad" onClick={handleButton}>
          <audio
            src="https://s3.amazonaws.com/freecodecamp/drums/Heater-1.mp3"
            className="clip"
            id="Q"
          ></audio>
          Q
        </button>

And Lastly here is my Controls Component:

class Controls extends React.Component {
  render() {
    return (
      <div id="drum-controls">
        <h4 id="display-label">Drum Sound</h4>
        <label id="display">{this.props.name}</label>
        <label id="volume-label">Volume: {this.props.volume} %</label>
        {/* React Slider. Doc: https://www.npmjs.com/package/rc-slider */}
        <Slider
          min={0}
          max={100}
          value={this.props.volume}
          onChange={this.props.sliderChange}
          id="volume-slider"
        />
      </div>
    );
  }
}

Hey @codyjamesbrooks, your vercel page doesn’t seem to be working. I’m getting some syntax errors in the console.

Thanks for the heads up!
I think I fixed it. Yes? No?
Sorry this is my first react deployment, so i’m still figuring that bit out.

Yes, it is working now. No worries, nobody expects perfection on the try :slight_smile:

(1). There is nothing wrong with colocating state and logic. On projects this size, it might not make much of an impact. But on more complex projects it’s always a good idea to not just dump everything in the root component.

(3). It can lead to memory leaks. Besides, there is no reason not to remove the event listener on unmount.

(4). I would suggest you keep index.js clean of code other than imports and the ReactDOM.render call. Use an App.js for the App component. Other than that I don’t think you really need more components for this project.

Thanks lasjorg!
I will add in an unmount to remove the listeners.
Good to know the standards for using index.js vrs. App.js

I really appreciate you taking a look. I write so many things that don’t get any eyes on them, and I have been starting to worry about reinforcing bad habits.

1 Like

No problem. I did look a bit at the code and have a few little things for you.

  1. Uppercase your component file names, i.e. give the files the same name as the components. So for example Controls.js.

  2. I would suggest you keep the state at the top of the constructor before any bindings. That is the most common location for it, so being consistent with that will help other people reading the code.

  3. Personally, I would name the handlers as handleActionOrEvent, it’s a common naming convention. So for example, onKeyPress would be handleKeyPress. In my opinion, the on designation should be reserved for the listeners in the JSX, e.g. onClick, or onSomeCustomEvent. You should also be consistent about the naming. So sliderChange should be handleSliderChange (or onSliderChange using your current naming convention). In the Drumpad component, you have handleButton so you do use the handle convention. However, here it is missing the “action”, so it would be handleButtonClick.

Anyway, just some little things to give you some more feedback. Keep up the good work.

1 Like

That is all great feedback, and exactly what I need at this point in my journey!

I hadn’t even considered that level of consistency with the language that I was using for listeners/action functions, But now that you mention it, I can see how it would improve my code clarity a lot.

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.